pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

Shaded Fraction on Sloped Terrains - PR1725 partial continuation

Open echedey-ls opened this issue 1 year ago • 36 comments

  • [x] Closes #1689
  • [ ] ~Closes #1690~
  • [x] Partially substitutes #1725
  • [x] I am familiar with the contributing guidelines
  • [x] Tests added
  • [x] Updates entries in docs/sphinx/source/reference for API changes.
  • [x] Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
  • [x] New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • [x] Pull request is nearly complete and ready for detailed review.
  • [x] Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

See #1725 for context.

Link to docs

I moved the linear loss model to #2004

Feel free to suggest anything and ask about the adopted solutions.

echedey-ls avatar Feb 03 '24 23:02 echedey-ls

Does, for example, a N-S SAT tracker azimuth follow the East - West direction or the torque tube? I would say the azimuth and elevation define the normal vector of any PV panel on it, but I'm not at all sure.

A distinction needs to be drawn between the angles that define the orientation of tracker and the angles that define the orientation of the modules on the tracker. A single-axis tracker with N-S axis has constant axis_azimuth=180 (or zero, but 180 is better). The modules on the tracker will have surface_azimuth=90 in the morning and surface_azimuth=270.

I will take a closer look at this PR in the coming days. Thanks for your patience @echedey-ls!

kandersolar avatar Feb 27 '24 20:02 kandersolar

Lot of thanks for clearing this up. Now I feel like it's obvious.

Don't worry about the review. I know it's premature to open a PR based on another one (I've got yet another one in the drawer that needs this implementation to work, sorry 😬 )

echedey-ls avatar Feb 27 '24 23:02 echedey-ls

I'm late jumping in here, but one thought I have: does this need to be limited to trackers? Or can the function name and variable names be updated to more generally cover fixed or tracking rows?

williamhobbs avatar Feb 28 '24 00:02 williamhobbs

It should work for fixed tilt just fine, so not limited to trackers

mikofski avatar Feb 28 '24 03:02 mikofski

Thanks, @mikofski. It seems like it would be helpful to change:

  • the function name from tracker_shaded_fraction to shaded_fraction,
  • the variables tracker_tilt and tracker_azimuth to surface_tilt and surface_azimuth, and
  • references in the docs to “trackers” to “rows”

or something like that.

williamhobbs avatar Feb 28 '24 03:02 williamhobbs

Maybe I misspoke. Sorry, I meant to say that it could be repurposed for fixed tilt with some care, but, I’m not sure if we should make it all purpose for both fixed tilt & SAT. For one, tracker azimuth and surface azimuth mean different things, so we can’t just swap them.

I think it would work if we treat fixed tilt as a tracker aligned east to west and set the tracker azimuth to either 90° or 270° measured clockwise from north, or 90° from the surface azimuth. Like a similar issue you raised recently, if azimuth were 90° then tilts would be negative if north and positive if south.

Currently this algorithm doesn’t seem to account for tracker axis tilt, but as I mentioned in the original PR #1725 I think it could be managed when calculating projected solar zenith? A tracker axis tilt would correspond to an east-west slope for a fixed tilt.

Anyway I apologize for being too glib, I should have been more careful in my replies, again sorry.

mikofski avatar Feb 28 '24 05:02 mikofski

I agree with @williamhobbs. surface_tilt and surface_azimuth terms seem to apply to all contexts independently of the used tracker, so they are less prone to be misunderstood by a user.

echedey-ls avatar Feb 28 '24 09:02 echedey-ls

Rebase conflicts solved. Ready for review!

echedey-ls avatar Mar 08 '24 16:03 echedey-ls

Reference in n

@echedey-ls could you update the code to use surface_x instead of tracker_x?

AdamRJensen avatar Mar 11 '24 21:03 AdamRJensen

@AdamRJensen I'm pleased to do so. Should I change the function name too?

echedey-ls avatar Mar 11 '24 21:03 echedey-ls

Beware, trackers_x doesn’t always mean the same as surface_x.

tracker_theta = surface_tilt only if tracker axis tilt is zero.

tracker_azimuth - 90° = surface_azimuth in the morning

tracker_azimuth + 90° = surface_azimuth in the afternoon

the terms are not interchangeable

mikofski avatar Mar 11 '24 23:03 mikofski

A few thoughts:

  • I propose renaming tracker_shaded_fraction to shaded_fraction1d. As pointed out above, the math is equally applicable to fixed-tilt arrays. And there are many ways of calculating a "shaded fraction" of an array, many of which calculate a 2-D area fraction instead of a 1-D length fraction like this equation does.
  • I think we need a more substantive reference. For the shaded fraction, doesn't the Slope-Aware Backtracking report solve the same problem? Is there another reference for the linear shade loss model? The SAM technical reference describes a linear loss model along these lines, but it deals with lost irradiance, not lost power...
  • We should not forget the question about whether the math uses left- or right-handed rotations. I think we should strive for pvlib to use right-handed rotations everywhere.
  • Just an FYI: @adamrjensen and I have a paper under review containing an improved shaded fraction equation that allows the tracker casting the shadow to be at a different rotation from the tracker receiving the shadow. It won't be published in time for 0.10.4, and I don't want to hold up this PR in the mean time, but to me it makes sense to plan to extend this function with some additional optional parameters in the near future.

kandersolar avatar Mar 12 '24 14:03 kandersolar

I propose renaming tracker_shaded_fraction to shaded_fraction1d.

+1 here

We should not forget the question about whether the math uses left- or right-handed rotations. I think we should strive for pvlib to use right-handed rotations everywhere.

++1 here.

cwhanse avatar Mar 13 '24 15:03 cwhanse

We should not forget the question about whether the math uses left- or right-handed rotations. I think we should strive for pvlib to use right-handed rotations everywhere.

++1 here.

Isn't that a bit hard with the way we use azimuth angles and positive heights above ground?

adriesse avatar Mar 13 '24 19:03 adriesse

I propose renaming tracker_shaded_fraction to shaded_fraction1d.

+1 here

Perhaps the one dimension should be identified/named?

adriesse avatar Mar 13 '24 19:03 adriesse

Isn't that a bit hard with the way we use azimuth angles and positive heights above ground?

I suppose what I said was overly broad and azimuth is a good example of a left-handed angle that we don't want to change, but I hope the spirit was clear: other functions in pvlib use a right-handed convention for module rotations and ground slopes, and it seems like we should keep to that convention in this PR. The coordinate system is half the battle with these geometric functions and we should be careful and intentional with our definitions.

Perhaps the one dimension should be identified/named?

Perhaps! Any suggestions?

kandersolar avatar Mar 13 '24 20:03 kandersolar

Perhaps the one dimension should be identified/named?

Perhaps! Any suggestions?

I'm probably not the best person to generate such a suggestions...

The doc says "The fraction of the collector width shaded by an adjacent row." I guess we could raise the question here as well as to whether it should be width, length, height, or whatever--naming is hard. I think something that suggests the shade line moves vertically would be best, maybe shaded_fraction_lower.

adriesse avatar Mar 14 '24 11:03 adriesse

I think something that suggests the shade line moves vertically would be best

Then vertical / horizontal as you say; only these two (frontal surface) dimensions can be shaded. And to be honest, the horizontal shade effect is of less importance compared to the vertical, so I don't think it is a priority to include it for now. Maybe in the future. I will keep the proposed shaded_fraction1d name and update the docs according to this possibility. If we end up with a shaded_fraction1d_horizontal we can consider changing (with a deprecation) the former name.

@kandersolar @cwhanse I'm applying your suggestions slowly, I need to fix the tests with the tracker_ -> surface_ changes.

echedey-ls avatar Mar 14 '24 17:03 echedey-ls

Ready for review 👀

Few doubts, notes and ideas:

  • I've changed the figure and added the reference of backtracking slope-aware, I hadn't seen the angle convention in this file. Since the implementation relies on the original Mikofski paper and notation, I've left it there with a comment.

  • The SAM technical reference describes a linear loss model along these lines, but it deals with lost irradiance, not lost power...

    The most common pattern for models is to modify the incident irradiance, right? Since the irradiance to power conversion is completely linear as the model, I lean toward just documenting that it can be applied to any of those values, with the respective updates to doctest examples.

  • an improved shaded fraction equation that allows the tracker casting the shadow to be at a different rotation from the tracker receiving the shadow

    I'd love to facilitate future implementation of it. How do you feel about making gcr and/or cross_axis_slope keyword-only parameters? That way the code will be more readable and new parameters can have their place after solar position, which I find as the best choice. They can be appended to the end of the function too, but I dislike having orientation params so far from each other.

Link to shaded_fraction1d docs.

echedey-ls avatar Mar 14 '24 20:03 echedey-ls

  • I think we need a more substantive reference. For the shaded fraction, doesn't the Slope-Aware Backtracking report solve the same problem? Is there another reference for the linear shade loss model?

@kandersolar, just to clarify, you are saying that we need a substantive reference that says linear power loss models are valid for thin film modules. Is that right?

If so, I would guess that First Solar published something on it. I found this https://energy.sandia.gov/wp-content/gallery/uploads/26-Littmann-Tracking-and-Diffuse-Shading.pdf0_.pdf, but that's maybe not as substantive as would be ideal. Maybe https://ieeexplore.ieee.org/abstract/document/6744267 (I don't have free access) or something else by Ngan (Nelson), Panchula, or Littman?

williamhobbs avatar Mar 14 '24 21:03 williamhobbs

@kandersolar, just to clarify, you are saying that we need a substantive reference that says linear power loss models are valid for thin film modules. Is that right?

Evidence and illustrations for thin-film aSi are provided in this paper: https://www.researchgate.net/publication/267261857_Non-Uniform_Conditions_and_Performance_in_Parallel-Only_and_SeriesParallel_Array_Configurations

The term "Linear loss" is another way of saying "no mismatch loss", it seems. I wouldn't claim this to be a adequately "substantive" reference, but the measurements were real.

adriesse avatar Mar 14 '24 23:03 adriesse

you are saying that we need a substantive reference that says linear power loss models are valid for thin film modules. Is that right?

What I'd really like to reference is a paper that deals with specifically the equation implemented in this PR: power_loss_fraction = shaded_fraction1d * (1 - diffuse_fraction).

Since the irradiance to power conversion is completely linear as the model

Only the simplest PV performance models assume that power is exactly linear with irradiance. Here's a demonstration of how the CEC model is nonlinear for a First Solar thin film module:

Click to expand!
import pvlib
import numpy as np
import matplotlib.pyplot as plt

cecdb = pvlib.pvsystem.retrieve_sam('cecmod')
module_params = cecdb['First_Solar__Inc__FS_6430']
module_params = module_params[['alpha_sc', 'a_ref', 'I_L_ref', 'I_o_ref', 'R_sh_ref', 'R_s', 'Adjust']]

effective_irradiance = np.linspace(0, 1200)
temperature_cell = np.array([25] * len(effective_irradiance))

sde_params = pvlib.pvsystem.calcparams_cec(effective_irradiance, temperature_cell, **module_params)
mpp = pvlib.pvsystem.max_power_point(*sde_params)

plt.plot(effective_irradiance, mpp['p_mp'] / effective_irradiance)
plt.ylabel('Pmp divided by Geff')
plt.xlabel('Geff')

So it does make a (small) difference whether the shaded fraction loss is applied to the irradiance or to the power.

Conceptually, applying the loss to the irradiance makes more sense to me. And that variant of the linear model has the SAM document as a reference. Should we switch to calling it an irradiance loss?

kandersolar avatar Mar 15 '24 12:03 kandersolar

Only the simplest PV performance models assume that power is exactly linear with irradiance

🤯 I’m not sure why I had never thought of it that way, but thanks for helping it “click” for me!

williamhobbs avatar Mar 15 '24 13:03 williamhobbs

There are a couple problems with this shaded fraction calculation. One is that the meaning of cross_axis_slope changes depending on surface_azimuth. That means that for a tracking system, you get problems like this, where a cross-axis slope increases (or decreases) the shading in both halves of the day instead of increasing in one half and decreasing in the other:

image

I think the only way to get expected behavior is for the user to calculate and pass in a time-varying cross_axis_slope. That seems like an unnecessary burden that will inevitable trip people up.

Another problem is that shaded fraction goes to zero when the sun goes below the slope plane. Seems to me to be better to keep it at 100% in those times, but maybe that's a matter of preference.

I think we should switch to calculating shaded fraction using the method in the slope-aware backtracking report. I think it would fix the above issues, and have the additional advantages of supporting nonzero axis_tilt and being more easily generalized in the future due to using tracker rotations and axis orientation angles instead of surface angles. @echedey-ls what do you think?

kandersolar avatar Mar 18 '24 14:03 kandersolar

@echedey-ls I think the issues mentioned above are big enough that they won't be resolved in time for v0.10.4, so I retagged this PR for v0.10.5.

Also, the generalized shaded fraction calculation I mentioned in https://github.com/pvlib/pvlib-python/pull/1962#issuecomment-1991805047 is now accepted by the journal. It might make sense for shaded_fraction1d to just implement that equation from the start instead of finalizing the current equation and then adding the generalized form as an extension later, since I think the latter will be extra work. Sorry for the confusing and conflicting guidance about this feature submission! At least we are learning useful things about how these functions should look like and how to document them well.

kandersolar avatar Mar 18 '24 16:03 kandersolar

Thanks Kevin for the in-depth review. I agree 100% on all your points. Maybe some tricks could be made by checking whether the Sun is on the front or the back of the array, but if you already have a paper accepted that's definitely better. Can you let me know once it gets published?

echedey-ls avatar Mar 18 '24 16:03 echedey-ls

I've changed the scope of this PR to only address the shaded fraction topic. The linear loss model will be left to another PR I'll open in the following days.

echedey-ls avatar Apr 01 '24 01:04 echedey-ls

Fixed a few things regarding the new implementation 🔧 . Ready to be re-reviewed!

echedey-ls avatar Apr 10 '24 17:04 echedey-ls

maybe we should include an Examples section with one or two basic cases (e.g. south-facing fixed tilt on flat terrain)?

That's an amazing idea. How do you feel about a gallery example, to replicate some 2-3 test cases in https://zenodo.org/doi/10.5281/zenodo.10513987 , per south-facing, north-facing, N-S HSAT configurations.

I say both south- and north- facing because I'd like to:

  • show examples with the different right-handed rotations explicitly (even thou they don't change too much)
  • avoid being the FOSS package taken as an example of the scientific bias 🙃 - I was responsible for it in the PSZA PR btw :grimacing:
    • south-facing yields 9 results
    • north-facing yields 0 results

echedey-ls avatar Apr 15 '24 22:04 echedey-ls

Sounds good to me, although I'm not sure the test cases in the zenodo archive are necessarily the best beginner examples (for example, there is no "simple" case where zL=zR=0), so maybe we should just invent some new example cases. Anyway let's add a gallery example in a separate PR.

Another question for this PR: I imagine most users will want to use this function with long time series inputs and get time series output back. With this function, when using different shaded and shading rotations, it is complicated by having to swap the two rotations depending on the time of day. Should the function perform that swapping automatically somehow? Should we just document it and put the burden on the user? Maybe @AdamRJensen has a comment?

kandersolar avatar Apr 16 '24 12:04 kandersolar