alan-turing-institute / deepsensor

A Python package for tackling diverse environmental prediction tasks with NPs.
https://alan-turing-institute.github.io/deepsensor/
MIT License
72 stars 15 forks source link

Optional dependencies for data downloaders #107

Closed davidwilby closed 6 months ago

davidwilby commented 6 months ago

Relates to #106 and #102

review-notebook-app[bot] commented 6 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

tom-andersson commented 6 months ago

This is great, thanks @davidwilby! I wasn't aware of importlib.util.find_spec. With the get-station-data import check I just tried to catch an ImportError, but importlib is probably more principled.

One thing before LGTM, I realised the user will bump into this error in the Data Requirements notebook before the DataProcessor one. Would you mind moving your note to that page? For example, just after the Data Sources paragraph.

Also left a comment on the paragraph you added that needs resolving.

davidwilby commented 6 months ago

This is great, thanks @davidwilby! I wasn't aware of importlib.util.find_spec. With the get-station-data import check I just tried to catch an ImportError, but importlib is probably more principled.

That would be my usual approach but my thinking here was that since deepsensor doesn't import rioxarray directly (only via xarray), it would avoid a sort of needless import. But we end up just importing importlib instead, maybe this is just fancier :shrug:

Arguably, using try/except and catching ImportError is more usual and possibly more readable. But I'd rather import a standard library module and barely use it than import a third-party dependency and not use it at all, since it will be more liable to possible instability/weirdness in the future.

One thing before LGTM, I realised the user will bump into this error in the Data Requirements notebook before the DataProcessor one. Would you mind moving your note to that page? For example, just after the Data Sources paragraph.

Absolutely, will do!

Actually I've opted to include it in both sections since I think that it's possible that users will begin with the User Guide section. (See d3a5892)

Also left a comment on the paragraph you added that needs resolving.

No problem, agreed.

tom-andersson commented 6 months ago

LGTM, thanks very much for these doc improvements @davidwilby!