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

Standardise variable name for clearsky GHI and DNI, add to glossary #2272

Open
RDaxini opened this issue Oct 20, 2024 · 6 comments
Open

Standardise variable name for clearsky GHI and DNI, add to glossary #2272

RDaxini opened this issue Oct 20, 2024 · 6 comments

Comments

@RDaxini
Copy link
Contributor

RDaxini commented Oct 20, 2024

Is your feature request related to a problem? Please describe.
I have counted three variations of clear sky GHI in the docs:

clearsky (in clearsky.detect_clearsky)
clearsky_ghi (in irradiance.clearsky_index)
ghi_clearsky (in irradiance.dirindex)
and no entry in the variables and symbols page (soon to become a glossary #2234)

for DNI, we have dni_clear in the variables and symbols page but also clearsky_dni (in irradiance.dni) and dni_clearsky (in irradiance.dirindex) in the docs.

Describe the solution you'd like

  • Agree a single format
  • Add/modify an entry in the variables and symbols page

Additional context
Related: #1253

@RDaxini RDaxini changed the title Standardise variable name for clearsky GHI and add to glossary Standardise variable name for clearsky GHI and DNI, add to glossary Oct 20, 2024
@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 20, 2024

Except for just clearsky, all options seem reasonable to me. I think either clearsky_xxx (written as you'd say it) or xxx_clear (shorter) would be best.

@cwhanse
Copy link
Member

cwhanse commented Oct 20, 2024

I prefer ghi_clear but it's a mild preference. Parallels the few terms that are a conditional irradiance, i.e., dni_clear, ghi_extra, poa_global, etc.

@RDaxini
Copy link
Contributor Author

RDaxini commented Oct 21, 2024

I think we're set on ghi_clear (also dni_clear for where it isn't already used)

Would updating these variables be classed as breaking changes and require some kind of deprecation? I am thinking about #2237
Is a separate PR required for each function that is changed, or is one PR for all the functions involving the changing variable appropriate? (e.g. #2273)

@adriesse
Copy link
Member

Did you consider having just one PR for these two names since the changes are so closely related?

@RDaxini
Copy link
Contributor Author

RDaxini commented Nov 29, 2024

Did you consider having just one PR for these two names since the changes are so closely related?

@adriesse I asked about PR structure here but thought each variable had to be separate, no response so I was not really sure. I confess I think I underestimated the overlap. Is it possible to combine PRs after creation? We should do that if it would help with merging later.

@adriesse
Copy link
Member

adriesse commented Dec 2, 2024

Did you consider having just one PR for these two names since the changes are so closely related?

@adriesse I asked about PR structure here but thought each variable had to be separate, no response so I was not really sure. I confess I think I underestimated the overlap. Is it possible to combine PRs after creation? We should do that if it would help with merging later.

Sorry I didn't speak up earlier. I'm not the best person to advise on the PR machinations. :(

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 a pull request may close this issue.

3 participants