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

Add netCDF4 as a dependency and install netcdf4 #429

Closed
wants to merge 6 commits into from

Conversation

seisman
Copy link
Member

@seisman seisman commented May 17, 2020

Description of proposed changes

xarray needs the netCDF4 package for reading (perhaps also writing) netCDF files. However, netCDF4 isn't installed by default since it's an optional dependency of xarray. Although pygmt doesn't import netCDF4 package explicitely, some features won't work without the package.

See #239 for past discussions.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

xarray needs the netCDF4 package for reading (perhaps also writing) netCDF files.
However, netCDF4 isn't installed by default since it's an optional
dependency of xarray. Although pygmt doesn't import netCDF4 package
explicitely, some features won't work without the package.

See #239 for past discussions.
@seisman seisman requested review from leouieda and weiji14 May 17, 2020 05:42
@seisman seisman changed the title List netCDF4 as a pygmt dependency Add netCDF4 as a dependency and install netCDF4 May 17, 2020
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Thanks @seisman, was surprised that netcdf4 wasn't listed! Just one minor comment on whether we should streamline the conda install instructions once #430 is merged in.

I do wonder whether we should list netcdf as a strict dependency, or have it as a (strongly) recommended one. For sure that conda users should install it and get it. But I wonder if there are advanced users that might not want to install it because of GPL issues (see intake/intake-xarray#41), though that would need to fixed on our setup.py.

@@ -79,7 +80,7 @@ First, we must configure conda to get packages from the
Now we can create a new conda environment with Python and all our dependencies installed
(we'll call it ``pygmt`` but you can change it to whatever you want)::

conda create --name pygmt python=3.6 pip numpy pandas xarray packaging gmt=6.0.0
conda create --name pygmt python=3.6 pip numpy pandas xarray netcdf4 packaging gmt=6.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Once #430 is merged, would it be better to just instruct users to conda install pygmt since it'll pull in all the side dependencies as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, perhaps we should merge conda-forge/pygmt-feedstock#2 first, then instruct users to install pygmt with:

conda create --name pygmt pygmt

@weiji14
Copy link
Member

weiji14 commented May 17, 2020

But I wonder if there are advanced users that might not want to install it because of GPL issues (see intake/intake-xarray#41), though that would need to fixed on our setup.py.

Then again, GMT is LGPL licensed, but PyGMT is BSD3 licensed, so I'm not sure if it matters. I'm not a lawyer though.

@weiji14
Copy link
Member

weiji14 commented May 18, 2020

Yeah. It falls back to a scipy netcdf3 implementation but the GMT files don't conform to that style so no luck.

_Originally posted by @leouieda in #239 (comment)

Just a note that xarray.open_dataset supports other 'engines' besides netcdf4 and scipy. See http://xarray.pydata.org/en/v0.15.1/generated/xarray.open_dataset.html?highlight=engine. For example, I've often used the h5netcdf engine (BSD3 licensed), and have been playing with the Zarr engine too (MIT licensed).

In the future, we might want to look into supporting other PyData 'array' type inputs (via some setting, e.g. as a kwargs) as an alternative to non-netcdf4 users, just not now in this PR 😄

@seisman seisman changed the title Add netCDF4 as a dependency and install netCDF4 WIP: Add netCDF4 as a dependency and install netCDF4 May 19, 2020
@weiji14 weiji14 added this to the 0.1.x milestone May 20, 2020
@seisman seisman changed the title WIP: Add netCDF4 as a dependency and install netCDF4 Add netCDF4 as a dependency and install netCDF4 May 21, 2020
@seisman
Copy link
Member Author

seisman commented May 21, 2020

@weiji14 I just moved netcdf4 to an optional dependency, and remove it from the conda install command.

@seisman seisman requested a review from weiji14 May 21, 2020 03:46
@seisman seisman changed the title Add netCDF4 as a dependency and install netCDF4 List netCDF4 in the optional dependencies May 21, 2020
@weiji14
Copy link
Member

weiji14 commented May 21, 2020

If we're going to put this as optional, then maybe remove it from setup.py too?

@seisman
Copy link
Member Author

seisman commented May 21, 2020

If we're going to put this as optional, then maybe remove it from setup.py too?

I personally prefer to have all dependencies installed.

AFAIK, pygmt.datasets.load_earth_relief() needs netCDF4 for working. Since this function is used in the tests and the tutorial, it's better to have it installed, especially for new Python users.

@weiji14
Copy link
Member

weiji14 commented May 21, 2020

Yeah, better to have it as a non-optional dependency then. We could have a pip install pygmt[minimal] setup in the future if anyone really doesn't want netcdf.

@seisman seisman changed the title List netCDF4 in the optional dependencies Add netCDF4 as a dependency and install netcdf4 May 21, 2020
@seisman
Copy link
Member Author

seisman commented May 21, 2020

Yeah, better to have it as a non-optional dependency then. We could have a pip install pygmt[minimal] setup in the future if anyone really doesn't want netcdf.

Done.

@seisman
Copy link
Member Author

seisman commented May 21, 2020

Merged into PR #430. Closing.

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.

2 participants