Conversation
Error handling has been optimized with the observer_position function, which now checks the type and value of the altitude, and raises appropriate exceptions in case of invalidity. Important astronomical constants are defined at the beginning of the file to improve code readability and maintainability. Type annotations have been added for better understanding and readability of the code.
djhoese
left a comment
djhoese
left a comment
There was a problem hiding this comment.
Nice attempt at tackling this. I had a couple suggestions and changes I'd like to see if we plan on merging this. Also note that I'm pretty in favor of annotations but others on the maintainer team may not enjoy all the clutter it adds. Especially for this module where so many types are possible as arguments.
| PERIHELION_DAY = 3 # Day of year with Earth in perihelion (approx.) | ||
|
|
||
| # Type alias for time arguments | ||
| TimeLike = Union[dt.datetime, np.datetime64, np.ndarray] |
There was a problem hiding this comment.
There is the numpy.typing module added in numpy 1.20 I believe. Could this be used to define the dtype of the ndarray? Using something like npt.ArrayLike[np.datetime64]?
| :param utc_time: UTC time | ||
| :type utc_time: datetime.datetime, numpy.datetime64, or array-like | ||
| :return: Days since year 2000 | ||
| :rtype: float or numpy.ndarray |
There was a problem hiding this comment.
I'm not sure if pyorbital has a defined docstring format it follows, but in Satpy we've been using Google style docstrings:
https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html
Would it be possible to reformat these to match? If other parts of pyorbital are using numpy style docstrings then I'd be open to that as well.
There was a problem hiding this comment.
Additionally, I think you can remove the type in the docstrings if you have the annotations. We'll need to possibly enable the correct sphinx options to copy the annotations to the rendered documentation, but that can come after everything else is sorted.
| def _days(dt: TimeLike) -> Union[float, np.ndarray]: | ||
| """Get the days (floating point) from *d_t*. | ||
|
|
||
| :param dt: Time delta | ||
| :type dt: datetime.timedelta or numpy.timedelta64 |
There was a problem hiding this comment.
Some of these type annotations and docstrings are incorrect I think. This function for example takes datetime.timedelta objects or a numpy array of timedelta64 objects. You have TimeLike listed here.
| A = 6378.137 # WGS84 Equatorial radius | ||
| # Astronomical constants | ||
| AU = 149597870700.0 # Astronomical unit in meters | ||
| EARTH_ flattening = 1 / 298.257223563 # Earth flattening WGS-84 |
There was a problem hiding this comment.
Typo here (extra space). Also why not uppercase for the flattening part?
4 participants
Error handling has been optimized with the observer_position function, which now checks the type and value of the altitude, and raises appropriate exceptions in case of invalidity. Important astronomical constants are defined at the beginning of the file to improve code readability and maintainability. Type annotations have been added for better understanding and readability of the code.
flake8 pyorbital