Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor ineichen function #199

Merged
merged 5 commits into from Jul 7, 2016
Merged

refactor ineichen function #199

merged 5 commits into from Jul 7, 2016

Conversation

wholmgren
Copy link
Member

I refactored the ineichen function to be a function of zenith, airmass, turbidity, ET irradiance, and altitude rather than time, latitude, longitude, and model options. Now, the ineichen function is a simple implementation of the published model, while the Location.get_clearsky method provides a simple way to get a time series of clear sky data for a particular lat/lon.

The refactoring also makes it possible to use higher dimensional data with the ineichen function, just like the new simplified_solis function. I hope to eventually make a new documentation page for all of this, but maybe you want to beat me to it.

I would need to write a few more tests before this can be merged.

Closes #155.

cc @DanRiley since we talked about this once before.

@wholmgren
Copy link
Member Author

I rewrote the tests for the simplified ineichen function and improved its nan handling. A couple of API things to consider:

  1. Should the linke_turbidity, dni_extra, and altitude parameters should have default values? The simplfied_solis function has default values for pw, aod, dni_extra, and pressure. It's probably best if they have similar default values.
  2. Should the ineichen (and simplified_solis) models accept solar zenith or solar elevation as an input? The ineichen function was originally written in terms of zenith, so that's what I used as a function argument. Later, I reread the Ineichen and Perez paper and discovered that the paper uses elevation, not zenith. The simplified solis paper and function is written in terms of elevation. Again, probably best if they're consistent.
  3. Ineichen is behind both the "ineichen" and the simplified solis models. Maybe there's a better name for the ineichen function... ineichenperez, ineichen_turbidity, ineichen_perez_linke_turbidity, kip (kasten, ineichen, perez). I'm ok leaving it alone, too.

@cwhanse
Copy link
Member

cwhanse commented Jul 6, 2016

1 – no strong opinion here, but it seems to me that dni_extra is the only one of the three that could have a reasonable default value.
2 – I’d pick a convention for pvlib-python and I don’t have strong feelings. Probably prefer whichever (zenith or elevation) is most convenient to obtain from the ephemeris functions.
3 – I’d leave the naming as is. If someone says ‘Ineichen model’ odds are they are referring to the model that we’ve coded rather than to one of the many other models he’s contributed to.

Cliff

@wholmgren
Copy link
Member Author

Thanks for the input, Cliff. Zenith is the more common parameter in pvlib and probably the broader solar energy literature. So, I'll leave this alone and consider changing the simplified solis arguments in a different issue/pull request. I'll also leave the function name as it is.

@wholmgren wholmgren merged commit 675454f into pvlib:master Jul 7, 2016
@wholmgren wholmgren deleted the ineichen branch July 7, 2016 18:57
@wholmgren
Copy link
Member Author

I forgot to say that I have some new clear sky documentation in the works, but readthedocs won't compile it right now. New PR coming when I get that figured out. In the meantime, here's a link to the new doc file:

https://github.com/wholmgren/pvlib-python/blob/csdocs/docs/sphinx/source/clearsky.rst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants