Check longitude and latitude are numbers (float or integer)

[ndclt]

• Closes #xxxx: I didn't find any issue linked to the modification
• I am familiar with the contributing guidelines
• Tests added
• Updates entries to docs/sphinx/source/api.rst for API changes. (not relevant for this MR)
• 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`). I didn't find a file for the unreleased version where I can add my modification
• 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 and Milestone are assigned to the Pull Request and linked Issue.

Context

In this Stackoverflow question, the OP creates a Location object with strings without errors and this leads to a Numpy error when using the function get_solarposition. Numpy couldn't add a string with two float array. The string was the observer_longitude directly given from the longitude attribute of a Location instance.

Goal

This PR aims at raising an error when something different from a number is given to the Location init function as longitude or latitude argument.

Implementation choice

I hesitate between the solution proposed in the PR and something like this:

# for longitude
try:
   self.longitude = float(longitude)
except ValueError:
   raise TypeError('Invalid longitude specification')

I prefer the PR version because it doesn't hide the conversion (which is done with the float use) but the PR version won't work with a size-1 numpy array whereas the float one will (but I don't know if it's a common use case).

[kandersolar]

Hi @ndclt, thanks for the PR, and answering the stackoverflow question :)

I'm not sure what I think about type validation here. In general pvlib-python doesn't do much if any type validation and relies on documenting the expected types in the docstring (see e.g. float for those parameters here). If we add type checks to lat & lon, should we add one for the altitude parameter as well? I'm not in favor of including manual type checks everywhere that someone might pass a string for something that needs to be numeric. Do you think lat & lon deserve an exception?

If we do add this, I think the error message should include some more detail like argument has type str but should be float.

[wholmgren]

Hi @ndclt, thanks for the PR, and answering the stackoverflow question :)

+1

I'm -1 on type validation but +1 on type annotations for the Location class. Type annotations are growing in python and I'd rather rely on that flexible tooling than building our own inflexible validators.

[ndclt]

Hi @kanderso-nrel and @wholmgren,

I'm not an regular user of pvlib (I discovered it with the question on stackoverflow and by the way, it was really easy to install it and test it before this PR). So, if you don't agree with this kind of check, I won't strongly push for it.

I could easily do a PR adding some type annotation to the __init__ function of the Location class:

class Location
    def __init__(self, latitude: float, longitude: float, tz: Union[str, datetime.timezone, datetime.tzinfo, int, float]='UTC', altitude: float=0, name: Optional[str]=None):
        ....

but as from what I see, there is any type in this library and that would be a really small begin. I never migrate an existing code base to annotation. I found this tips from the mypy checker.

I also give a try to the monkeytype package in this branch. Would you like me to create a PR with these modifications?

I answer your questions below (but more for the record than changing your mind).

Do you think lat & lon deserve an exception?

I was thinking so because of the side effect. It's not so easy to guess that a 6 depth stacktrace is coming from a wrong given type when creating the Location instance. The timezone was also checked.

The full stacktrace of the stackoverflow question

Traceback (most recent call last):
File "/home/my-home/Dev/stackoverflow/stackoverflow/pv_lib.py", line 36, in <module>
    solpos = location_object.get_solarposition(times)
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/location.py", line 191, in get_solarposition
    return solarposition.get_solarposition(times, latitude=self.latitude,
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/solarposition.py", line 114, in get_solarposition
    ephem_df = spa_python(time, latitude, longitude, altitude,
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/solarposition.py", line 375, in spa_python
    spa.solar_position(unixtime, lat, lon, elev, pressure, temperature,
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/spa.py", line 1150, in solar_position
    result = do_calc(unixtime, lat, lon, elev, pressure,
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/spa.py", line 1056, in solar_position_numpy
    H = local_hour_angle(v, lon, alpha)
File "/home/my-home/.local/share/virtualenvs/stackoverflow/lib/python3.9/site-packages/pvlib/spa.py", line 734, in local_hour_angle
    H = apparent_sidereal_time + observer_longitude - sun_right_ascension
numpy.core._exceptions.UFuncTypeError: ufunc 'add' did not contain a loop with signature matching types (dtype('<U32'), dtype('<U32')) -> dtype('<U32')

If we add type checks to lat & lon, should we add one for the altitude parameter as well?

Yes I think we should.

[cwhanse]

I'm also -1 on type validation and neutral about using type annotations, mostly because I don't really understand them and am not clear what advantage they provide over the docstring, other than the convenience of raising hints in an IDE.

[wholmgren]

I also give a try to the monkeytype package in this branch. Would you like me to create a PR with these modifications?

I previously wrote type hints for Location.__init__ (and other objects) but never got to the point of creating a PR. The relevant commit for Location is wholmgren@5f1b7ad. If I recall correctly, making mypy happy required a bit more than the monkeytype package suggests.

Some in the pvlib community are not enthusiastic about type hints, so to get started I would not annotate the other methods. Simply annotating the object constructors/attributes should catch a fair number of problems.

other than the convenience of raising hints in an IDE.

To me they are most helpful when the IDE is setup to flag/underline conflicts as I write/review code. Most of the pvlib function layer is straightforward enough, but the object layer could benefit from hints. I don't recall the details, but the ModelChainResult annotations helped identify a few bugs during that big refactor. Annotations are relatively helpful to people that write applications (like we did for Solar Performance Insight) or production scripts but less helpful (or worse) for people that write research code. pvlib serves both communities so it's hard to know the right answer. I'm not aware of any good tooling to combine type hints with documentation, so there's a bit of duplication involved.

In any case, it seems there's not an appetite for type checking these inputs. I'd be happy for help with experimenting with type annotations in the Location/PVSystem/Mount/ModelChain __init__ methods and setting up mypy in the CI, but I'm not confident the efforts will ultimately be accepted.

[ndclt]

From all the comments I get, I close the pull request.

Thanks to all of you for the review.