Skip to content

Update default transposition model #1861

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

Open
AdamRJensen opened this issue Sep 15, 2023 · 7 comments
Open

Update default transposition model #1861

AdamRJensen opened this issue Sep 15, 2023 · 7 comments
Milestone

Comments

@AdamRJensen
Copy link
Member

Having the default transposition model be isotropic seems like a very poor choice. Isotropic models are known to have very large biases and should only be used in very specific use cases (generally not be new users).

I did a brief survey of commercial software and found that the vast majority only offer the Hay-Davirs and Perez model, with the 1990 Perez model seeming to be favored as the default model.

It would seem to make sense to have the same default model used in pvlib.modelchain.ModelChain.

def get_total_irradiance(surface_tilt, surface_azimuth,
solar_zenith, solar_azimuth,
dni, ghi, dhi, dni_extra=None, airmass=None,
albedo=0.25, surface_type=None,
model='isotropic',
model_perez='allsitescomposite1990'):

@AdamRJensen
Copy link
Member Author

I suppose this would seen as a "breaking change" and would have to wait until 0.11.0 if we choose to do this.

@cwhanse
Copy link
Member

cwhanse commented Sep 18, 2023

I agree with changing the default. IIRC there is some work by Matt Lave that showed Hay-Davies was mildly better when one starts with satellite-based GHI, and Perez better if starting with ground measured GHI. Either is OK with me as a better default.

@kandersolar
Copy link
Member

It would be great if get_total_irradiance could still be run with the current minimal set of inputs, even if the default model is changed to something more sophisticated. #1225 got us halfway there by automatically calculating airmass for Perez, but left ET DNI alone since there is not always a datetime index available to calculate it. Maybe we could examine the inputs to see if a datetime index is available and only raise the error if not?

@mikofski
Copy link
Member

mikofski commented Sep 18, 2023

I’m fine with using a solar constant as default for ET DNI. For example either 1367[Wm-2] or 1400[Wm-2] would be fine as a placeholder for a more exact value imo

@AdamRJensen AdamRJensen added this to the 0.11.0 milestone Sep 18, 2023
@AdamRJensen
Copy link
Member Author

It would be great if get_total_irradiance could still be run with the current minimal set of inputs, even if the default model is changed to something more sophisticated. ... Maybe we could examine the inputs to see if a datetime index is available and only raise the error if not?

This seems like a good approach to me. I suppose it should be incorporated here:

if (model in {'haydavies', 'reindl', 'perez'}) and (dni_extra is None):
raise ValueError(f'dni_extra is required for model {model}')

What would be the best way of assessing if the index is datetime? Something like this:

isinstance(d.index, pd.DatetimeIndex)

@kurt-rhee
Copy link
Contributor

I would like to throw my vote in the hat for Perez as the default transposition model, about 90% of predictions run in PlantPredict use the Perez model (this is without regard to the source of the meteorological data). This is an imperfect measure of true usage industry wide, but I think it is a nice data point.

@kandersolar
Copy link
Member

I suppose this would seen as a "breaking change" and would have to wait until 0.11.0 if we choose to do this.

Seems like we can and should do a deprecation period for this first. That can be implemented any time, so I've removed the v0.11.0 milestone from this issue.

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

No branches or pull requests

5 participants