Skip to content

Add "ape" to variables_style_rules.csv #2164

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

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Conversation

RDaxini
Copy link
Contributor

@RDaxini RDaxini commented Aug 13, 2024

Adds "ape" to the user guide variables and symbols page to complement the new function created in PR #2140 , which itself was created to close Issue #2135

AdamRJensen
AdamRJensen previously approved these changes Aug 14, 2024
Copy link
Member

@AdamRJensen AdamRJensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit that ape stands out as the only acronym from the variable list that I wouldn't intuitively know what was prior to these last PRs.

Edit: given that ape is only used once in the entire documentation (in the output of the average_photon_energy function then I don't think it warrants being added here (yet). I think we should use average_photon_energy to the extent that we can, as ape does not see universal usage in the community or widespread usage in pvlib like say GHI, DNI, etc.

@AdamRJensen AdamRJensen dismissed their stale review August 14, 2024 08:21

I retract my review

@RDaxini
Copy link
Contributor Author

RDaxini commented Aug 14, 2024

Edit: given that ape is only used once in the entire documentation (in the output of the average_photon_energy function then I don't think it warrants being added here (yet). I think we should use average_photon_energy to the extent that we can, as ape does not see universal usage in the community or widespread usage in pvlib like say GHI, DNI, etc.

It depends what you mean by universal usage. The initialism "ape" for "average photon energy" might not be known by everyone, but this is to be expected since not everyone works with every metric. I think the spectrum in particular is also a less common area of study. As for instances where the average photon energy is used, I am certain that this initialism is used universally in the PV performance modelling community. Happy to be corrected if you have found exceptions to this.

That's my understanding of the general subject, but as to whether an entry into the pvlib user guide is necessary I will leave that up to the more experienced pvlib maintainers --- @AdamRJensen @cwhanse (or another vote) if there is consensus to hold off on this addition then I'm happy to close this PR. At the moment it's 1 approval 1 non-approval.

@cwhanse
Copy link
Member

cwhanse commented Sep 11, 2024

@pvlib/pvlib-maintainer any objection to merging?

Copy link
Member

@adriesse adriesse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no harm done here.

@AdamRJensen AdamRJensen merged commit 5aa9ba1 into pvlib:main Sep 11, 2024
31 checks passed
@RDaxini RDaxini deleted the variable_ape branch September 11, 2024 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants