pvlib-python icon indicating copy to clipboard operation
pvlib-python copied to clipboard

More common terms for solar zenith and solar azimuth

Open cwhanse opened this issue 3 years ago • 18 comments

In pvlib, two solar zenith quantities are of interest: true (geometric) zenith angle, and apparent (refraction corrected) zenith angle. Currently, zenith, solar_zenith and apparent_zenith all appear in different places, and the meaning of each isn't consistent throughout. For example, solarposition/spa_c uses zenith (true) and apparent_zenith (refraction-corrected), and Location methods expect these terms. But both apparent_zenith and solar_zenith are used in SingleAxisTracker methods (the latter without being specific in the docstring), and in places where I think refraction-corrected zenith should be expected, e.g., PVSystem.get_aoi.

For solar azimuth, there is not counterpart to true vs. refraction-corrected. But for the solar azimuth, both azimuth and solar_azimuth are being used.

Describe the solution you'd like

  1. Agreement on the pvlib meaning of zenith and solar_zenith. apparent_zenith's meaning is clear. The other two terms leave room for uncertainty.

  2. Agreement on one term to use for solar azimuth: azimuth or solar_azimuth.

Describe alternatives you've considered It makes sense to have a term that is not specific to true or apparent zenith. Perhaps zenith for this use, and solar_zenith specifically for true zenith?

cwhanse avatar Feb 13 '22 21:02 cwhanse

Thanks for raising this issue. I have noticed the various terms usage as well, and have been careful to read the docs (especially in the irradiance and atmosphere modules) to make sure to use what I think is supposed to be the correct intended input.

In the spirit "explicit is better than implicit" I prefer to use the solar_ prefix because there are other azimuths like the orientation of the PV system, which might not be perfectly N-S (180°). Also I've always been a little confused by the term apparent b/c I hadn't heard it before. In my experience with the SOLPOS and SPECRL2 codebases they both use the term "refracted zenith" which to me seems more explicit than "apparent" which I think could mean either the geometric extraterrestrial zenith or the zenith refracted through the atmosphere.

I've been open to relearning the terms used for azimuth and zenith, and I'll agree to whatever consistent terms we use. I also agree that consistency throughout pvlib is important. I think it might also be useful to try to be consistent with other tools as well like SOLPOS, SPECTRL2, rdtools, SAM, and others.

mikofski avatar Feb 14 '22 02:02 mikofski

I also like the solar_ prefix. I'd prefer to refer to the apparent/refracted zenith as solar_zenith. solar_apparent/refracted_zenith is fine too, though. Perhaps the current "zenith" becomes solar_unrefracted_zenith? I don't like "true".

wholmgren avatar Feb 14 '22 03:02 wholmgren

Good initiative.

adriesse avatar Feb 14 '22 13:02 adriesse

pv-terms recommends apparent_solar_zenith. I have the sense that apparent zenith is the more useful quantity, which to me suggests that it might deserve the unqualified solar_zenith.

We should also fix the instances of apparent_azimuths around pvlib (solarposition.pyephem, tracking.singleaxis, perhaps others).

kandersolar avatar Feb 14 '22 16:02 kandersolar

First of all I think it would be great to agree on a common definition.

Also, I agree with @kanderso-nrel that apparent zenith is more commonly used and it would be nice to simply call it solar_zenith. This way basic users need not consider which to use. In either case, the docstring should always clearly specify whether it is refraction corrected or not.

The dillema is also an issue in regards to variable mapping of the iotools as mentioned here. I'm convinced that many of the iotools probably map refraction-corrected (apparent) zenith angle to solar_zenith.

Here's a list of relevant mapping instances in the iotools module: PVGIS maps to solar_elevation, CAMS to solar_zenith, SOLRAD to solar_zenith, SURFRAD to solar_zenith, and soon PSM3 to apparent_zenith.

AdamRJensen avatar Feb 16 '22 23:02 AdamRJensen

If I understand the comments correctly, here's a proposal for a consensus vote:

  • solar_zenith means apparent (refraction-corrected) solar zenith.
  • solar_elevation to likewise mean apparent solar elevation.
  • solar_zenith_geom means unrefracted zenith : tossing the term out here for comment/vote
  • zenith is discouraged and will be replaced by solar_zenith unless solar_zenith_geom is specifically required
  • solar_azimuth to be used instead of azimuth
  • apparent_azimuth to become solar_azimuth not sure why we have apparent_azimuth to begin with

cwhanse avatar Feb 17 '22 19:02 cwhanse

I wonder if geometric zenith even needs its own name if it's not commonly needed and is only very slightly different from refracted zenith. Maybe call both of them solar_zenith and rely on the docstring to indicate if geometric is needed?

kandersolar avatar Feb 17 '22 20:02 kandersolar

IDK folks, this seems to contradict pv-terms, maybe we should start there? PEP0020 says there should preferably be only one <name>, and Phil Karlton says naming is the hardest thing. Maybe we shouldn't barrel into this.

Also are there any instances of where the input is ambiguous? For example, in get_relative_airmass what should the input be called? This function can take either apparent/refracted zenith or the geometric/extraterrestrial zenith depending on the model.

mikofski avatar Feb 17 '22 21:02 mikofski

this seems to contradict pv-terms,

I don't have a problem with that, personally.

The urgency is to settle on some terms to get #717 merged, and it would be nice if the terms moved pvlib toward a consistent end state.

Good point about get_relative_airmass, that appears to be a case where a generic term zenith is needed.

cwhanse avatar Feb 17 '22 21:02 cwhanse

Other options could be solar_zenith_unrefracted or solar_zenith_nonrefracted - both of which are descriptive and unambiguous.

I'm a little wary of _geom because it's a shortening of a full word and... aren't all angles geometric?

I'm also not a huge fan of apparent due to its rather vague meaning - looking the word up in the dictionary didn't exactly help:

apparent: seeming real or true, but not necessarily so

I agree with @mikofski that we shouldn't rush this and perhaps not hold off any PRs ready for merging.

AdamRJensen avatar Feb 17 '22 21:02 AdamRJensen

Two separate objectives seem to be:

  • distinguish between solar angles with and without refraction (only zenith)
  • distinguish between solar angles and some other angles like surface (especially azimuth)

For the first, I think in the majority of situations there is no strict requirement for one or the other, but it is rather a question of how accurate (or pedantic) one wishes to be. So I think an agnostic label would be best in most places.

adriesse avatar Feb 17 '22 23:02 adriesse

We have a precedent for solar_zenith_analytical in the analytical solar position functions. I can't remember why we preferred "analytical" vs. "geometrical". Also at the risk of bike shedding, I do prefer when the descriptor leads the variable as apposed to being a subscript. EG: analytical_solar_zenith vs. solar_zenith_analytical or the worst of all zenith_solar_analytical yuk. The first rolls of the tongue almost as if you were talking in normal conversation.

mikofski avatar Feb 18 '22 04:02 mikofski

So are we in agreement with https://github.com/pvlib/pvlib-python/issues/1403#issuecomment-1043363825 except for the geom/analytical name?

I generally agree with @mikofski on the descriptor order but we already have the solar_zenith_analytical function and I think consistency with that is a plus. If we are still looking for a more descriptive name, the function doc string mentions "spherical trigonometry" so maybe solar_zenith_spherical (or spherical_solar_zenith).

Thinking we should finalize this soon so that we can make a 0.9.1 release.

wholmgren avatar Feb 28 '22 19:02 wholmgren

I am willing to draft a PR. If we are changing parameter names, what's the machinery for deprecation? Add the new name and retain the old name, raising a warning if the old name is used?

cwhanse avatar Mar 02 '22 21:03 cwhanse

The vast majority of these will be positional arguments, in which case I say no deprecation is needed. If there are any keyword arguments then yes, you've got the right idea (pretty sure you did this for some functions in the last year or two).

We could also consider holding off on making any changes until 0.10. Simply agreeing here on the terms would allow #1374 to move forward, give us confidence in the choices made in #717, and let us move forward with the 0.9.1 release.

wholmgren avatar Mar 02 '22 21:03 wholmgren

What about column names in returned dataframes? I don't see a reasonable way to detect usage of the old names in return values. Adding the new names alongside the old doesn't seem satisfactory. Maybe the least bad option is to just change the names and lean on the fact that we aren't 1.0 yet.

kandersolar avatar Mar 02 '22 21:03 kandersolar

Ohhh yea great point @kanderso-nrel. Changing column names might be the most user-hostile thing we've ever done. I think we should do it and it should be in 0.10.0 rather than 0.9.1. I can imagine returning a hacky object that wraps the DataFrame like

class _Awful:
    def __init__(data):
        self.data = data
    def __getitem__(key):
        if key in ['apparent_zenith', ...]:
            warnings.warn()
        return data[key]
# similar for getattr or getattribute

def get_solarposition():
    # result = call calculators
    return _Awful(result)

We did something like this for ModelChain attributes when switching to ModelChainResult. But it's a lot more... ambitious... to wrap a DataFrame.

wholmgren avatar Mar 02 '22 21:03 wholmgren

I would cast a vote for the prefix true to be used with solar_zenith when a distinction is needed. The zenith angle without adjustment for refraction does indeed point to the true position of the sun relative to the observer. Another option could be astronomical.

adriesse avatar Mar 03 '22 20:03 adriesse