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

[WIP] Implement engerer2 decomposition model and threlkeld-jordan clear-sky model #1366

Closed
wants to merge 2 commits into from

Conversation

kandersolar
Copy link
Member

  • Closes Add engerer2 decomposition model #1316
  • I am familiar with the contributing guidelines
  • Tests added
  • Updates entries to docs/sphinx/source/api.rst for API changes.
  • 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`).
  • New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
  • Pull request is nearly complete and ready for detailed review.
  • Maintainer: Appropriate GitHub Labels and Milestone are assigned to the Pull Request and linked Issue.

I took a stab at implementing the engerer2 decomposition model. Its reference suggests using it with the threlkeld-jordan clear-sky model, so I implemented that as well. However, on comparing this implementation with the code published for the engerer2 reference, an issue in the original emerged (JamieMBright/Engerer2-separation-model#2) that might call the reference's coefficients into question. Unclear at this point whether we'll want to merge this, but at the suggestion of @AdamRJensen I'm opening this PR so that at least this implementation is available in the meantime.

@kandersolar kandersolar marked this pull request as draft December 31, 2021 15:36
@adriesse
Copy link
Member

adriesse commented Jan 3, 2022

Good detective work!

@AdamRJensen
Copy link
Member

In case it is decided not to implement the engerer2 model, is it perhaps worth still implementing the Threlkeld-Jordan clear-sky model? Especially given that it has been incorrectly implemented previously by others, I think that it would be great having a reference of it in pvlib.

@kandersolar
Copy link
Member Author

Yes, agreed, the clear-sky model is not corrupted by the engerer2 coefficient issue. For context: engerer2 is a model that predicts diffuse fraction using what is essentially a logistic regression on a few input parameters, including clear-sky GHI modeled using this T-J model. However, it seems that the recommended coefficient values to the engerer2 logistic regression were fit to ground station data using an implementation of the T-J clear-sky model with a minor flaw, meaning the fitted coefficients are likely flawed to some extent as well. I don't think we can estimate the degree of the error without significant effort, so I'm hesitant to merge engerer2. But the underlying clear-sky model is still fine.

Also from what I was seeing in some older references this model might be better called some kind of ASHRAE model instead of T-J. Need to look a bit more into that.

@cwhanse
Copy link
Member

cwhanse commented Jan 7, 2022

@kanderso-nrel have you located a copy of the 1958 T-J paper?

@kandersolar
Copy link
Member Author

kandersolar commented Jan 7, 2022

This is my (tenuous) understanding of how the equations implemented in this PR's threlkeld_jordan function came to be:

  1. In 1957, T-J published clear-sky solar irradiance curves. Here is a link to a paper (that I'm not wholly convinced is the right one), courtesy of @AdamRJensen: https://archive.org/details/sim_heating-piping-air-conditioning-engineering-hpac_1957-12_29_12/page/134/mode/2up
  2. In 1965, Stephenson proposed a clear-sky irradiance model structure, without prescribing specific values for its coefficients. https://www.sciencedirect.com/science/article/pii/0038092X65902070
  3. In 1967, Stephenson published monthly values for those coefficients that were "selected so that the resulting values of Idn would be in close agreement with the values given by Threlkeld and Jordan". https://nrc-publications.canada.ca/eng/view/ft/?id=02c8ad55-4607-439c-b098-135fee140d2a
  4. In 2005, Masters fit sinusoids to the monthly values, which the Engerer2 paper used to find A, k, C. Eq 7.22 in G Masters "Renewable and Efficient Electric Power Systems"

It seems that ASHRAE adopted the model and the monthly coefficients somewhere along the way as well, but I don't have a link handy for that.

@kandersolar
Copy link
Member Author

My understanding regarding flawed coefficients in the engerer2 decomposition model remains unchanged, but it seems that the TJ clear-sky model is obscure enough that I probably won't ever get the motivation to add it on its own. In that case, no need to keep this PR open. It's there if anyone wants to someday pick it up and write tests.

@JamieMBright
Copy link

I could probably pick up the bugs in the engerer2 model and refit it if of interest. I was pretty gutted to learn i had a flawed TJ model. Not my finest hour!

@adriesse
Copy link
Member

adriesse commented Feb 7, 2023

I could probably pick up the bugs in the engerer2 model and refit it if of interest.

I think it is of interest, and you get another publication! :)

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

Successfully merging this pull request may close these issues.

Add engerer2 decomposition model
5 participants