-
Notifications
You must be signed in to change notification settings - Fork 1k
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
variables_style_rules.csv could use an update #1253
Comments
Add |
I really dig this idea. Ensuring consistent naming is one place we're not doing great in and this would be a good help I think. |
I understand this PR is talking about updating the user guide Variables and Symbols page entries with what is used in pvlib, but after running the script I also found there are multiple duplicate parameter names in slightly different forms, e.g. Do you guys think it's worth a PR to unify these names? I think it would be worth it at least to make the function parameters consistent with what we already have in the user guide. I could review the list in more detail and make a start on this in a PR. Unifying parameter names that aren't already in the user guide could be a separate piece of work as that may require more discussion to reach a consensus on a name. |
It would be great to see a list of parameter name variants you found. In the past, a deprecation process has been followed when a parameter name has changed. I don't think we want to rename in one large PR. We've done that in small bites. #894 and probably other issues are relevant. |
It's a painstaking process cross checking everything (parameter map, user guide, function docstring) so the table below is incomplete, but it gives a few examples 😅 I understand there may be some reasons for some of the differences (I haven't tracked every related issue / definition at this stage) but I did check most definitions at least, e.g. "current" is definitely the same as "photocurrent" in this case (not short-circuit current or anything else). The table below is a summary of what I have found. I hope it helps. Note this does not include terms that aren't in the user guide that potentially should be, just terms for which there is an overlap either between a function and the user guide, or between functions (with or without a user guide entry)
*not sure whether it's ac or dc, not obvious unless you dive into the references/source code, either way there's a still a duplicate somewhere The starred points are a few examples of where terms require further clarification. Some definitions are even misleading (in my opinion) e.g. There are also instances where two parameters have a similar meaning but have no overlap, e.g.
Ah of course, I had not thought about the required deprecation process but that does make sense. Good point, thanks for explaining. |
|
The table of parameter names in the docs is a bit dusty and doesn't perfectly reflect today's pvlib. To make it easier to figure out where the gaps are, here's a script that scans the package, finds the parameter names currently in use, and compares with that table:
unlisted_names
has over 300 elements. I think it makes sense to leave most of them out of the table, for example function-specific names likeupper_line_length
andu0
, but a few of the unlisted ones are common enough to warrant inclusion, e.g.albedo
. It also shows a few inconsistencies likedelta_t
vsdeltaT
andpoa_irradiance
vspoa_global
.An idea, maybe a bad one: once we've figured out which of the unlisted parameter names we don't want to include in the table, put them in an "ignore list" and create a CI check that performs the above scan to check for new parameter names that aren't in the table but maybe should be.
The text was updated successfully, but these errors were encountered: