initial implementation of pvsyst_celltemp function (#552)

[CameronTStark]

• Closes issue ENH: Implement PVSyst Cell Temperature Model #552
• I am familiar with the contributing guidelines.
• Fully tested. Added and/or modified tests to ensure correct behavior for all reasonable inputs. Tests (usually) must pass on the TravisCI and Appveyor testing services.
• Updates entries to docs/sphinx/source/api.rst for API changes.
• Adds description and name entries in the appropriate docs/sphinx/source/whatsnew file for all changes.
• Code quality and style is sufficient. Passes LGTM and SticklerCI checks.
• New code is fully documented. Includes sphinx/numpydoc compliant docstrings and comments in the code where necessary.
• Pull request is nearly complete and ready for detailed review.

[CameronTStark]

General FYI, I started with the code @mikofski presented and implemented aspects from sapm_celltemp() for some consistency.

A potential concern is that I implemented different temp_models keys than what sapm_celltemp() uses to mirror the default options presented for PVSyst on the PVLIB website. This may end up being confusing when using the same PVSystem instance to execute both pvsyst_celltemp() and sapm_celltemp() and limit future options for running both concurrently. Let me know if there are other preferred options or, instead, if we should include more independent PVSyst-centric options.

[cwhanse]

I think the temp_model keys you are using are fine. They trace back to two defaults that PVsyst offers: see the Array Thermal losses page of the PVsyst v6 help files.

Be sure to update api.rst and whatsnew.rst.

Thanks for the contribution!

[CameronTStark]

I've updated the docs @cwhanse, thanks for your guidance!

[cwhanse]

@CameronTStark could you Resolve conversation for the formatting items you've addressed? Thanks.

[wholmgren]

@cwhanse @CameronTStark why the closing and reopening? can we leave it open until merged?

[wholmgren]

Thanks @CameronTStark!

I made a handful of minor comments below (hoping that this still works with the PR closed).

Nice job on the tests.

[CameronTStark]

@cwhanse @CameronTStark why the closing and reopening? can we leave it open until merged?

Sorry, I hit the Comment and close button by accident.

[CameronTStark]

I made a handful of minor comments below (hoping that this still works with the PR closed).

@wholmgren it looks like the PR isn't updating with my latest commit now that its closed again. Maybe it'll update if it's reopened?

[cwhanse]

I also must have mis-clicked and closed it.

[wholmgren]

@CameronTStark can you try to resolve the merge conflict? Then I think it's ready to merge.

[cwhanse]

Thanks for the contribution @CameronTStark

[CameronTStark]

Thanks for the contribution @CameronTStark

Sure thing and thanks for the easy introduction to contributing here.

If there's another place to contribute next that makes sense from here please let me know. If not I'll just check issues again and dive in.

[cwhanse]

Good comment, @mikofski, Mark<mailto:Mark.Mikofski@dnvgl.com> I hadn’t considered that. Easy issue and PR, both welcome. From: Mark Mikofski <notifications@github.com> Sent: Tuesday, December 11, 2018 2:39 PM To: pvlib/pvlib-python <pvlib-python@noreply.github.com> Cc: Hansen, Clifford W <cwhanse@sandia.gov>; State change <state_change@noreply.github.com> Subject: [EXTERNAL] Re: [pvlib/pvlib-python] initial implementation of pvsyst_celltemp function (#552) (#628) @mikofski commented on this pull request.

________________________________ In pvlib/pvsystem.py<#628 (comment)>:

+

+ Returns + ------- + temp_cell : numeric or Series + Cell temperature in degrees Celsius + + References + ---------- + [1]"PVsyst 6 Help", Files.pvsyst.com, 2018. [Online]. Available: + http://files.pvsyst.com/help/index.html. [Accessed: 10- Dec- 2018]. + + [2] Faiman, D. (2008). "Assessing the outdoor operating temperature of + photovoltaic modules." Progress in Photovoltaics 16(4): 307-315. + """ + + temp_models = {"freestanding": (29.0, 0), "insulated": (15.0, 0)} My thoughts in case this is ever revisited, sorry for commenting so late. * Move this constant to the module level. IMO avoid internally scoped constants. The computer needs to find memory for this every time this function is called. If it's defined at the module level then I think this memory will only be allocated once, when the module is imported the first time. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub<#628 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFJNLzUEc_NbGcuSybs0wDy2UPmKpN6xks5u4CXhgaJpZM4ZHfAi>.

[mikofski]

@CameronTStark in the future maybe consider duck typing instead of type checking. Even though duck typing is the Pythonic preference, it may not always work best. I think your solution here is fine, but in general I think duck typing usually results in more robust code. Eg:

else:
    natural_convenction_coeff, forced_convection_coeff = temp_model
    # already raises ValueError or TypeError
    # or use try: except to raise a more descriptive exception

[CameronTStark]

@mikofski Yes, thanks for the suggestion! I've been trying to pay attention more to duck typing in my code and thought about it for this instance but since this was my first contribution and sapm_celltemp() had the Look Before You Leap type check in it already I figured I'd just follow convention.

[wholmgren]

@CameronTStark thanks for this good pull request. A related contribution would be to add PVSystem and ModelChain wrappers for this new function. I suggest making a new issue for that feature if you want to pursue it.

[cwhanse]

#633

[CameronTStark]

@CameronTStark thanks for this good pull request. A related contribution would be to add PVSystem and ModelChain wrappers for this new function. I suggest making a new issue for that feature if you want to pursue it.

Sounds good. I'll get on it. Thanks!

[cwhanse]

@CameronTStark thanks! Just a note, the method tests should verify that the object methods call the underlying function, there's not a need to retest the function's output.

[adriesse]

The variable absorption_coeff has a very misleading name. It is in fact the portion of the incident irradiance that is converted to heat.

[adriesse]

On a more general note, does this model originate with PVsyst?

There are several variations of the same model that differ only in the way they define and/or determine the coefficients. Would we like to have completely separate functions for these variations? Or rather one core function and some helper functions for the coefficient calculations? Or some other organization?

[adriesse]

Incidentally, these questions arose while I was discussing with my student intern which model he should code for his first contribution to pvlib-python.

[cwhanse]

@adriesse can you point me to an example of the variations? I'm not as familiar with this family of temperature models as I'd like to be. If I understand correctly, the underlying model is Faiman's, and PVsyst implements a variation of it. As best we know, pvsyst_celltemp implements the function described in PVsyst user manual.

I can see value in Faiman_celltemp and other functions (and now I'm regretting not asking for an API change to name this family celltemp_pvsyst celltemp_sapm etc.) I'd prefer to see a different function for each model, where the function for a model variation can call helpers etc. to implement the variation (akin to the way the dirint DNI model is implemented).

[cwhanse]

The variable absorption_coeff has a very misleading name. It is in fact the portion of the incident irradiance that is converted to heat.

I see it as the portion of irradiance that is absorbed by the cell, either as heat or converted to current. In either case absorption coefficient is the term used in the PVsyst documentation so it seems helpful to retain the name in pvlib.

[adriesse]

That one is called alpha_absorption. Check line 1964.

[cwhanse]

That one is called alpha_absorption. Check line 1964.

You are correct, I see the issue now. We have a parameter named alpha_absorption ( = 0.9) which is referred to in the PVsyst documentation as Alpha

Alpha is the absorption coefficient

We have an internal variable absorption_coeff which is a product of three factors, one being Alpha, and which has no name in PVsyst. It is confusing to call this product absorption_coeff because, well, its not a coefficient (it has units of temperature), and PVsyst uses phrase "absorption coefficient" to refer to one factor, alpha.

PR welcome to clarify the naming.

[adriesse]

Regarding model variations: The determination of cell/module temperature using an energy balance equation and a simple heat transfer coefficient "U" goes way back. PVsyst, SAM, TRNSYS, Duffie & Beckman, Faiman all do it, and I expect there are others. Making part of the U value proportional to wind speed is old too: D&B makes reference to a 1954 textbook, and certain constants from that book persist in SAM today despite the fact they were derived for a 0.5m2 plate.

The variations principally arise from assumptions regarding the fraction of energy absorbed, the relative importance given to wind speed, and various adjustments (direct or indirect) to the U values for mounting.

[adriesse]

We have an internal variable absorption_coeff which is a product of three factors, one being Alpha, and which has no name in PVsyst. It is confusing to call this product absorption_coeff because, well, its not a coefficient (it has units of temperature),

Actually, absorption_coeff has the units of irradiance.

[cwhanse]

Actually, absorption_coeff has the units of irradiance.

Sigh, then the other variable combined_convection_coeff must have units of W/m2 K. We should add units to the docstring.