Skip to content

MRG: refactor modules into netCDF4 package #409

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

Merged
merged 2 commits into from
May 9, 2015

Conversation

matthew-brett
Copy link
Contributor

Move netCDF4.so and netCDF4_utils into a netCDF4 package.

Closes : #408

@matthew-brett
Copy link
Contributor Author

I'm not sure what to do about the generated C files - these seem to include the paths with which I configured the build, when I build them with Cython, and that seems like a bad idea. I'm probably not understanding something about building the C files.

@matthew-brett matthew-brett force-pushed the refactor-package-structure branch 2 times, most recently from 9667e0f to 42f8f19 Compare May 6, 2015 07:11
@matthew-brett
Copy link
Contributor Author

I think I have found that building the c files involves:

  • Deleting the Cython meta section at the top of the file;

  • Making sure you are on a system where the generated constants.pyx is

    DEF HAS_RENAME_GRP = 0
    DEF HAS_NC_INQ_PATH = 0
    DEF HAS_NC_INQ_FORMAT_EXTENDED = 0
    

    I was building the C files on my laptop, with a relatively recent version of netCDF4 so HAS_NC_INQ_PATH in particular got set to 1, meaning that, when travis-ci runs the build against an older version of the netCDF4 library, there are undefined symbols. I think.

Did I miss a utility to build the default generated C file? Given the library dependencies are fairly heavy, maybe it would be reasonable to make Cython an install-time dependency?

@matthew-brett matthew-brett force-pushed the refactor-package-structure branch from 42f8f19 to 6ee5133 Compare May 6, 2015 07:30
@jswhit
Copy link
Collaborator

jswhit commented May 6, 2015

that's right, you should start with all the values 0 in constants.pyx, then setup.py will check the HDF5 API to see which features are available and set entries to 1 if they are. netCDF4.c is then generated from netCDF4.pyx by cython. If you just want to use the defauilt constants.pyx and netCDF4.c (build without any of the newer API features and don't require cython), you can set use_cython=False in the setup.cfg file.

I don't think you need to remove the docstrings at the top of netCDF4.pyx.

@matthew-brett
Copy link
Contributor Author

For the constants.pyx - my problem was I was trying to generate the C files to be committed into the repo. So I did want to use Cython (to generate the C files) but I didn't want constants.pyx to be autogenerated on my system, otherwise the C files would assume HDF5 features that might not be present on the target system - such as travis-ci. In the end I had to generate the C files on another system that I had that happened to have an HDF missing these three features.

@matthew-brett
Copy link
Contributor Author

You probably saw, I moved the docstring from the Cython module to the __init__.py, just because that is usually where the top-level docstring goes. Do you want me to move it back?

@jswhit
Copy link
Collaborator

jswhit commented May 7, 2015

I guess it's OK there, as long as epydoc still finds it. Unfortunately, epydoc now doesn't work - running create_docs.sh gives

+------------------------------------------------------------------------------------
| In /Users/jsw/python/pull409/netCDF4/init.py:
| No documentation available!
| Error: Import failed:
| ImportError: No module named _netCDF4 (line 767)
| Error: Source code parsing failed:
| Error during parsing: invalid syntax (/Users/jsw/python/pull409/netCDF4/
| init.py, line 767) -- Bad dotted name
|
This sounds related to JonnyJD/epydoc-m#6

Why did you leave netCDF4_utiis.py?

the utilities need to be modified to use netCDF4.utils instead of netCDF4_utils.

Also, a Changelog entry would be nice.

@jswhit
Copy link
Collaborator

jswhit commented May 7, 2015

I've been meaning to move away from epydoc, since it's no longer maintained. It looks like that may have to happen in order to do this.

@shoyer
Copy link
Contributor

shoyer commented May 7, 2015

@jswhit I'm pretty sure @matthew-brett left around netCDF4_utils.py to preserve backwards compatibility.

On the other hand, we don't actually document direct use of netCDF4_utils anywhere, and it's mostly private utilities for the netCDF4-python module (I think), so we may not need that.

@matthew-brett
Copy link
Contributor Author

Yes, I left netCDF4_utils.py in case someone was using it. I guess you could leave it in place with a Deprecation warning for one release cycle? I don't know how adventurous your users are :)

I don't think there are any remaining uses of that module, but please correct me if I'm wrong.

Could you use sphinx / autodoc for the documentation?

@shoyer
Copy link
Contributor

shoyer commented May 7, 2015

If you look in netCDF4_utils, you'll notice that it's all private functions -- except for a few recently added (last month) functions that correspond to command line scripts. I think it's perfectly safe to remove it -- really the entire module should have been private.

@matthew-brett
Copy link
Contributor Author

Fine by me to remove it. The tests import some private functions, which in fact come from the utils module, and I've put these imports into the __init__.py because the tests import from netCDF4 - should these also be removed? Or does the fact that the tests import them mean that they may be used by other people?

@jswhit
Copy link
Collaborator

jswhit commented May 7, 2015

All of the stuff in netCDF4_utils.py is private, and is not used outside of the tests and the utilities.

@jswhit
Copy link
Collaborator

jswhit commented May 7, 2015

I have a workaround for the epydoc problem.

@jswhit
Copy link
Collaborator

jswhit commented May 7, 2015

The workaround for epydoc involves moving the docstring out of init.py and back into _netCDF4.pyx.

@matthew-brett
Copy link
Contributor Author

OK - so am I right in thinking that

  • docstring should go back into netCDF4.pyx;
  • delete netCDF4_utils.py
  • remove the utils.py underscore imports used by the tests from __init__.py and import them directly in the tests using them

is the right way to go?

@jswhit
Copy link
Collaborator

jswhit commented May 8, 2015

Yes, and in addition modify the utilities (ncinfo, nc3tonc4, nc4tonc3) to import from netCDF4.utils instead of netCDF4_utils.

...and add a Changelog entry, and it should then be ready to merge.

@matthew-brett matthew-brett force-pushed the refactor-package-structure branch 2 times, most recently from 47fdf8c to fc4edb0 Compare May 8, 2015 22:41
@matthew-brett
Copy link
Contributor Author

OK - I think I have done all that, let me know if I missed something.

@matthew-brett matthew-brett force-pushed the refactor-package-structure branch from fc4edb0 to 1566346 Compare May 9, 2015 01:07
@jswhit
Copy link
Collaborator

jswhit commented May 9, 2015

a minor suggestion - move utils.pyx (perhaps rename to _utils.pyx) into netCDF4 directory and rename 'src' directory 'include'.

Also, the create_docs.sh script needs to be changed to

epydoc -v --no-frames --no-private --introspect-only --name netcdf4-python -o docs netCDF4._netCDF4

@matthew-brett
Copy link
Contributor Author

Isn't utils.pyx already in the netCDF4 directory?

@matthew-brett
Copy link
Contributor Author

Ah no - sorry - I haven't had enough coffee - and I was thinking of utils.py.

I think I am right in thinking that utils.pyx is an include-only file? So there is no corresponding module. In that case, I would have thought putting it into the netCDF4 directory would be confusing, especially as there is already an utils.py in that directory.

* Make netCDF4 a package;
* Move netCDF4, netCDF4_utils into package;
* Make compatibility wrapper module for netCDF4_utils;
* Rebuild c files from moved pyx files.
Python build products to .gitignore

[skip ci]
@matthew-brett matthew-brett force-pushed the refactor-package-structure branch from 5bb0b26 to eeb504f Compare May 9, 2015 19:17
@matthew-brett
Copy link
Contributor Author

I renamed 'src' to 'include' and added the doc build line.

@jswhit
Copy link
Collaborator

jswhit commented May 9, 2015

good enough - thanks! I'll go ahead and merge. After the merge, I'll move all the code from utils.pyx to _netCDF4.pyx - it seems redundant to have the code split into two directories.

jswhit pushed a commit that referenced this pull request May 9, 2015
MRG: refactor modules into netCDF4 package
@jswhit jswhit merged commit 2300ee0 into Unidata:master May 9, 2015
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.

Consider moving netcdf4.so into own package?
3 participants