Lookup altitude2

[nicomt]

• Closes Altitude lookup table #1516
• I am familiar with the contributing guidelines
• Tests added
• Updates entries in docs/sphinx/source/reference for API changes.
• Adds description and name entries in the appropriate "what's new" file in docs/sphinx/source/whatsnew for all changes. Includes link to the GitHub Issue with :issue:`num` or this Pull Request with :pull:`num`. Includes contributor name and/or GitHub username (link with :ghuser:`user`).
• New code is fully documented. Includes numpydoc compliant docstrings, examples, and comments where necessary.
• Pull request is nearly complete and ready for detailed review.
• Maintainer: Appropriate GitHub Labels (including remote-data) and Milestone are assigned to the Pull Request and linked Issue.

In this pull request, I fix the outstanding issues from the last pull request and replace functions to use lookup_altitude by default

[nicomt]

The coverage for the spa_c solar position method keeps failing even though it is covered in tests.
Could that be caused by a dependency issue? Like the spa.c and spa.h files missing from the test?

[kandersolar]

The coverage for the spa_c solar position method keeps failing even though it is covered in tests. Could that be caused by a dependency issue? Like the spa.c and spa.h files missing from the test?

Yep, we don't include the SPA C code in the pvlib repository and tests due to licensing issues (#9). Any CI coverage issues around SPA C can/should be ignored.

[kandersolar]

@nicomt we're on the cusp of releasing 0.9.3, which includes your last PR. Do you agree that there is no problem making that release without including this PR? Since some defaults are changing, I think it makes sense to wait for 0.10.0 here, just want to confirm that the previous PR can stand on its own without this one.

[nicomt]

@kanderso-nrel You are right the last release should stand on its own. I don't see any problem delaying this PR.

[AdamRJensen]

Took a brief look and everything seems to check out.

Still a little skeptical about relying on an external file, which may be updated in the future and cause inconsistencies between versions.

[kandersolar]

Thanks again @nicomt for this PR. I think I like the Location class looking up its own altitude, but I'm not sure it's a good idea to also do the lookup inside the pvlib.solarposition functions. It's okay for the classes to have a little "magic" under the hood if it makes things more convenient, but I think we want more transparency in the functions. I'm also nervous about the (probably small) runtime cost of the lookup, since most people will not be specifying altitude themselves. I lean towards keeping the current static defaults in the pvlib.solarposition.* functions, but I'm curious what other maintainers think about it.

We'll also need a "what's new" entry, but let's decide where to go with this PR first.

[cwhanse]

I lean to agree with @kandersolar: leave the default altitude to be sea level in the solarposition functions.

[nicomt]

Those concerns are valid, and leaving sea level as the default seems reasonable.
I can close this PR, but then that leaves us with a few options:

1. We make the altitude lookup automatic for the Location class only.

loc = Location(latitude, longitude)  # altitude == location.lookup_altitude
spa_c(times, latitude, longitude) # altitude == 0

2. We integrate the altitude lookup everywhere but make it more explicit

loc = Location(latitude, longitude)  # altitude == 0
spa_c(times, latitude, longitude) # altitude == 0
loc = Location(latitude, longitude, altitude="lookup")  # altitude == location.lookup_altitude
spa_c(times, latitude, longitude, altitude="lookup")  # altitude == location.lookup_altitude

3. We make the altitude lookup explicit and only available for the Location class.

loc = Location(latitude, longitude)  # altitude == 0
loc = Location(latitude, longitude, altitude="lookup")  # altitude == location.lookup_altitude
spa_c(times, latitude, longitude)  # altitude == 0

5. We leave it as it is and let the user pass the altitude manually

loc = Location(latitude, longitude, altitude=location.lookup_altitude(latitude, longitude))
spa_c(times, latitude, longitude, altitude=location.lookup_altitude(latitude, longitude))

What do you guys think?

[nicomt]

Closing for now.
While we decide what to do.

[cwhanse]

@nicomt my vote is #1, let Location lookup altitude, but require it explicitly for the solar position functions.