Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Iceflow with icepyx notebook #50

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

trey-stafford
Copy link
Contributor

@trey-stafford trey-stafford commented Oct 30, 2024

Resolves #29


📚 Documentation preview 📚: https://iceflow--50.org.readthedocs.build/en/50/

Copy link
Member

@asteiker asteiker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will commit updates I made while I was reviewing. For iceflow-example, I cleared the code outputs because I ran into errors with my install. For iceflow-with-icepyx I tried not to actually run the cells but I accidentally did on atl06 resampling block.

Overall these notebooks are excellent and most of my comments, other than the install issues, are just minor suggestions to improve the flow and written guidance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General thought: bounding box parameters are not always consistent across Earthdata libraries but are generally simplified or modeled after CMR like earthaccess. This could be an enhancement in the future to change to something more streamlined like

BBOX = bounding_box(-103.125559, -75.180563, -102.677327, -74.798063)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good observation, I created an issue here to address: #51

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for awareness, I pip installed nsidc-iceflow in the Openscapes 2i2c Jupyter Hub and got back these dependency conflicts:

ERROR: pip's dependency resolver does not currently take into account all the packages that are installed. This behaviour is the source of the following dependency conflicts.
numba 0.59.1 requires numpy<1.27,>=1.22, but you have numpy 2.1.2 which is incompatible.
pyarrow 15.0.2 requires numpy<2,>=1.16.6, but you have numpy 2.1.2 which is incompatible.
scikit-learn 1.4.1.post1 requires numpy<2.0,>=1.19.5, but you have numpy 2.1.2 which is incompatible.
statsmodels 0.14.1 requires numpy<2,>=1.18, but you have numpy 2.1.2 which is incompatible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not able to run this further due to the numpy issue:

A module that was compiled using NumPy 1.x cannot be run in
NumPy 2.1.2 as it may crash. To support both 1.x and 2.x
versions of NumPy, modules must be compiled with NumPy 2.0.
Some module may need to rebuild instead e.g. with 'pybind11>=2.12'.

If you are a user of the module, the easiest solution will be to
downgrade to 'numpy<2' or try to upgrade the affected module.
We expect that some modules will need time to support NumPy 2.
```

It seems like perhaps we should not be supporting the old version of NumPy in IceFlow but I don't know exactly what is being used in the library that depends on the older version. 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separate install error:

ImportError: cannot import name 'BoundingBox' from 'nsidc.iceflow.data.models' ([/srv/conda/envs/notebook/lib/python3.10/site-packages/nsidc/iceflow/data/models.py](https://openscapes.2i2c.cloud/srv/conda/envs/notebook/lib/python3.10/site-packages/nsidc/iceflow/data/models.py))

docs/iceflow-example.ipynb Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm reviewing the iceflow-with-icepyx notebook, I wonder if it's worth mentioning the new dask function that could be used to perform a larger request? Maybe just a reference to the API docs or the icepyx notebook itself for more info?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note to the beginning of the example notebook about the iceflow-with-icepyx notebook:

To learn about how to download and manage larger amounts of data across many datasets with nsidc-iceflow, see the Using iceflow with icepyx to Generate an Elevation Timeseries notebook.

dd75b99

I'm also going to add some higher-level docs about the library and supported datasets. That seems like a gap right now.

docs/iceflow-with-icepyx.ipynb Show resolved Hide resolved
notebooks/iceflow-example.ipynb Show resolved Hide resolved
notebooks/iceflow-with-icepyx.ipynb Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Develop example showing iceflow and icepyx working together
2 participants