ihesp / ipart Goto Github PK

The documentation makes the design purpose of IPART clear, but the target audience should also be described as a further component of the 'statement of need' criterion for inclusion of a project in JOSS.

Please, towards the openjournals/joss-reviews#2407 review, provide an outline of the intended users for IPART. This can be very brief, e.g. a single sentence. As an example, I had a look at some packages with published JOSS papers and found a good example here for geemap, namely the paragraph starting (though a similar sentence alone would be sufficient):

geemap is intended for students and researchers, who would like to utilize the Python ecosystem of diverse libraries and tools to explore Google Earth Engine.

Also, though I wouldn't count any of these as compulsory to satisfying the criteria above, I think it would be really useful to:

1. have the project description from the documentation, i.e:

   IPART (Image-Processing based Atmospheric River Tracking) is a Python package for automated Atmospheric River (AR) detection, axis finding and AR tracking from gridded Integrated Vapor Transport (IVT) data, for instance Reanalysis datasets.

   (or perhaps even more from the 'Introduction section') at the top of the README file, as well as the start of the documentation. I believe it is common for people to check the README to understand the basics about a project. I certainly do that most of the time.

2. provide a link to the documentation from the 'About' section of the project repo (see for example the netcdf4-python repo homepage, so it can be accessed without having to go quite far down in the README to find the link.

This issue is in reference to my JOSS review of your paper (openjournals/joss-reviews#2407).

I am trying to understand the recommended installation procedure for this package.

On one hand, you seem to provide a pip package with dependencies:

On the other hand, your installation instructions don't mention pip and instead recommend manually creating a new conda environment from scratch. The instructions are mostly about how to create conda environments. This does not adhere to best practices for distributing python code. For example, what if you want to use this package in an existing environment? Do you have to create a new environment?

A better solution would be to provide working pip and / or conda-forge packages which automatically check for and, if needed, install the required dependencies on installation. This would allow the user to simply run pip install ipart or conda install -c conda-forge ipart. Could you provide a pip or conda-forge package for this software?

A related issue is whether the user is required to clone the master repository in order to use the software. I notice that there are various scripts and notebooks that are part of the repo but not the package itself. Are these required or optional. Again, this is not a standard practice for scientific python software. I recommend that users only be required to interact with the git repository if they wish to contribute to IPART development. Otherwise a pip / conda install should be sufficient to use the software.

Making these changes would also help resolve #2.

This is copying from comments made by @sadielbartholomew. See original post at openjournals/joss-reviews#2407 (comment).

Continuing on the topic of improvements that are not compulsory towards acceptance in the paper given the open criteria, but would be good to think about going forwards, for good practice with metadata I suggest making more use of the CF Conventions (the recommended standard for netCDF), namely as described in the three points below.

1. Increasing the compliance of the datasets included in the repository to the CF Conventions, especially those under the notebooks directory which users may interact with if they try out IPART with the provided Notebooks. Notably both uflux_s_6_1984_Jan.nc & vflux_s_6_1984_Jan.nc provided there are marked by global attribute as being CF-compliant to CF 1.6:

    :Conventions = "CF-1.6" ;

which is okay (relative to the ideal, latest version, 1.8), but immediately I see improvements in compliance that could be made.

For example, the variable & dimensions are all described by a long_name attribute, where use of a standard_name attribute is preferable as each is unambiguous (see e.g. here). The time, lat & lon coordinates can take standard names of the same identifier as currently used for the long name, and from a quick search on the names table for "eastward" AND "vapor" I think the data itself with long_name=Vertical integral of eastward water vapour flux and units kg m**-1 s**-1 could probably be assigned a standard name of eastward_atmosphere_water_vapor_transport_across_unit_distance, or similar.

2. Rephrasing aspects of the 'Data preparation' section of the documentation in terms of terminology from the CF Conventions. For instance, instead of stating there that:

    Source data are the u- and v- components of the vertically integrated vapor fluxes, in a rectangular grid.

you could explicitly state the standard names of data variables which would be applicable, e.g. something similar to northward_... and eastward_atmosphere_water_vapor_transport_across_unit_distance and maybe link to the definition of all grids which may be considered rectangular, for clarity. This would make it crystal clear whether a user's dataset(s) may be appropriate for processing by IPART.

3. Making code changes to accommodate conventions. In particular, the required ordering of dimensions for IPART seemingly go against conventions given that the 'Data preparation' section states that: "the user is responsible for making sure that the data are saved in the following rank order: (time, level, latitude, longitude)" but as conveyed in this section "The CF convention places no rigid restrictions on the order of dimensions, however we encourage data producers to make the extra effort to stay within the COARDS standard order" where "COARDS restricts the axis (equivalently dimension) ordering to be longitude, latitude, vertical, and time".

So, you are advocating that users define data dimensions in the inverse order to that recommended. To make IPART more immediately accessible, you could amend your code so that it accepts the outlined conventional order, rather than the inverse.

I couldn't correctly install CDAT modules. Any advice on how you did that?
I share a colab notebook with what I have tried with the conda list showing that cdms2 and cdutil but can't import them.

https://colab.research.google.com/drive/1wQhTRY0yY327eat9VB2HkPQIWeOxPUGm?usp=sharing

I noticed the sidebar in the documentation lists out the contents of only one section, 'Main functionalities', whereas the full documentation has numerous important sections at that top level of the contents tree, as illustrated (compare the sidebar and the Table of Contents listings) by this screenshot:

I think it would be much more helpful to readers for navigation purposes to, in the sidebar, instead list all of those top-level sections out with one or two more sub-levels of the tree for each.

(As noted when reviewing towards openjournals/joss-reviews#2407.)

Hello developers,
I installed the ipart=3.6.1 using conda under python=3.10,
When I test the functionality of this package using "compute_ivt.py" under the "scripts" directory. The calculation procedure is fine, but it shows the following plotting error:
AttributeError: 'Axes' object has no attribute 'get_geometry'.
The corresponding package version is also attached, cartopy=0.21.1, matplotlib=3.7.2.
I think it may be related to the update of the relevant packages that abort the old use of the "get_geometry" method. Please check it.

This issue is in reference to my JOSS review (openjournals/joss-reviews#2407)

Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support

I didn't find this information in your docs. However, it does appear to be in your README.

If you encounter problems or would like to help improve the code, please don't hesitate to fire up an issue or pull request.

This is a pretty terse contributor guide. You may wish to elaborate it a bit.

Dear author, I wonder if it is convenient for you to provide the results of atmospheric rivers identification, or I wonder if it is convenient for you to provide the IVT data of CMIP6. Thank you very much!

I wanted to check to see if the notebooks and scripts programs are the same and if I can run either one.

Best wishes!

_FillValue error when trying to save the nc files using funcs.saveNC () . It seems a _FillValue attribute needs to be set

funcs.saveNC(abpath_out, ivt.'w')

AttributeError                            Traceback (most recent call last)
<ipython-input-36-b3979129d066> in <module>
----> 1 funcs.saveNC(abpath_out, ivt)

c:\users\e806872\ipart\ipart\utils\funcs.py in saveNC(abpath_out, varNV, mode, dtype)
    816 
    817     with Dataset(abpath_out, mode) as fout:
--> 818         saveNCDims(fout, varNV.axislist)
    819         _saveNCVAR(fout, varNV, dtype)
    820 

c:\users\e806872\ipart\ipart\utils\funcs.py in saveNCDims(fout, axislist)
    896         for kk,vv in aa.attributes.items():
    897             if kk!='isunlimited':
--> 898                 axisvar.setncattr(kk, vv)
    899 
    900     return

netCDF4\_netCDF4.pyx in netCDF4._netCDF4.Variable.setncattr()

AttributeError: _FillValue attribute must be set when variable is created (using fill_value keyword to createVariable)

This happened also when the running the THR script

File "/usr/local/lib/python3.6/site-packages/ipart/thr.py", line 278, in rotatingTHR
    funcs.saveNC(abpath_out, var1, 'w')
  File "/usr/local/lib/python3.6/site-packages/ipart/utils/funcs.py", line 818, in saveNC
    saveNCDims(fout, varNV.axislist)
  File "/usr/local/lib/python3.6/site-packages/ipart/utils/funcs.py", line 898, in saveNCDims
    axisvar.setncattr(kk, vv)
  File "netCDF4/_netCDF4.pyx", line 4123, in netCDF4._netCDF4.Variable.setncattr

AttributeError: _FillValue attribute must be set when variable is created (using fill_value keyword to createVariable) 

Hi. Though the API documentation is quite detailed, towards the openjournals/joss-reviews#2407 review, I have noticed two aspects that I think are barriers towards the 'Documentation -> Functionality documentation' review requirement being deemed satisfactory:

1. There are some parameters for a number of methods in the IPART API that are not documented (see examples below);
2. Not all of the util modules, and not all methods from those that are present, are documented. This is indicated via '(selected parts)' after the names of the utils.funcs.py & utils.plot.py pages in the page listings, but there is no explanation as to why the modules under utils in the directory hierarchy are only partially documented.

Examples for (1)

For example (though it may be the case for other modules so please check), looking at the API reference pages for thr.py and AR_detector.py I notice that:

• the verbose parameter appears in the signature of many methods but is not documented in any of them (though perhaps by name quite obvious in purpose, I think it is still best to state it so users can be sure);
• thr.rotatingTHR is stated to accept a parameter kernel but that is not described;
• no parameters are documented for AR_detector.cart2Wind, AR_detector.plotGraph or AR_detector.wind2Cart despite their signatures suggesting they all accept parameters;
• AR_detector.prepareMeta has parameters ntime, nlat, nlon which are only described in reference in the documentation of other parameters;
• AR_detector.findARs has the parameter param_dict documented, but it does not appear in the signature for that method, so it is not clear whether it is indeed a parameter that is accepted (or has any influence) on it.

Resolution

Please can you address the following points to resolve the above:

• If a module is not documented, or certain functions within it are not, please either:
  • document these (if you are intending to but have not yet got this done); or
  • provide a brief explanation as to why they are deliberately not being documented, e.g for utils/funcs.py you could just state something like (guessing a potential reason), "Only some functions from this module are documented. The other undocumented functions are intended as private functions, not to be exposed to the user."
• For all methods that you do document, ensure all parameters appearing in the signature are explicitly (i.e. not just referenced in the description for other parameters) described. This includes the examples outlined above, but they are only from the two pages referenced, so it is not an exhaustive list to address.

Thanks.

The scripts under scripts/ rely on hard-coded filesystem paths, e.g. from compute_ivt.py:

#--------------Globals------------------------------------------
#-----------uflux----------------------
UFLUX_FILE='/home/guangzhi/datasets/erai_qflux/uflux_m1-60_6_2007_cln-cea-proj.nc'
UFLUX_VARID='uflux'

#-----------vflux----------------------
VFLUX_FILE='/home/guangzhi/datasets/erai_qflux/vflux_m1-60_6_2007_cln-cea-proj.nc'
VFLUX_VARID='vflux'

OUTPUTFILE='/home/guangzhi/datasets/quicksave2/THR/ivt_m1-60_6_2007_crop2.nc';

and therefore when any user other than yourself tries to run them they run into obvious errors relating to those files not being found, for example I observe:

$ python compute_ivt.py 
Traceback (most recent call last):
  File "compute_ivt.py", line 37, in <module>
    ufluxNV=funcs.readNC(UFLUX_FILE, UFLUX_VARID)
  File "/home/sadie/IPART/ipart/utils/funcs.py", line 775, in readNC
    fin=Dataset(abpath_in, 'r')
  File "netCDF4/_netCDF4.pyx", line 2358, in netCDF4._netCDF4.Dataset.__init__
  File "netCDF4/_netCDF4.pyx", line 1926, in netCDF4._netCDF4._ensure_nc_success
FileNotFoundError: [Errno 2] No such file or directory: b'/home/guangzhi/datasets/erai_qflux/uflux_m1-60_6_2007_cln-cea-proj.nc'
$ python detect_ARs.py
Traceback (most recent call last):
  File "detect_ARs.py", line 187, in <module>
    quNV=funcs.readNC(UQ_FILE_NAME, UQ_VAR)
  File "/home/sadie/IPART/ipart/utils/funcs.py", line 775, in readNC
    fin=Dataset(abpath_in, 'r')
  File "netCDF4/_netCDF4.pyx", line 2358, in netCDF4._netCDF4.Dataset.__init__
  File "netCDF4/_netCDF4.pyx", line 1926, in netCDF4._netCDF4._ensure_nc_success
FileNotFoundError: [Errno 2] No such file or directory: b'/home/guangzhi/datasets/erai_qflux/uflux_m1-60_6_2007_cln-cea-proj.nc'

I appreciate your intention was to include these as templates, but I think in this form, where they rely on resources not included in the repo and reference your personal paths and hence error immediately, they are not very useful to users. This is in contrast to the notebooks which are very useful as there is no user-specific stipulation and all of the datasets are provided in the repo, so everything should work for anyone, as they did when I tested them, assuming they have installed IPART and dependencies and have the right general environment.

To allow users to make good use of the scripts, I suggest adding new datasets to the repo that can be pointed to via those variables such as UFLUX_FILE that users can use to run the scripts on, with some brief guidance stating what users should change to point to their own datasets, etc., instead, to explore the scripts and capability.

(As noted when reviewing towards openjournals/joss-reviews#2407.)

Towards openjournals/joss-reviews#2407 review, I am trying to confirm a working installation and sufficient claimed functionality but, after a standard install as per the current instructions, I am running immediately into the same exception, ImportError.

Namely, using the following commands in the root directory of my fork of the IPART repo, updated to the current state of the IPART master branch:

$ conda env create -f environment_py3.yml
...
$ conda activate ipartpy3
(ipartpy3) $ pip install -e .
...
(ipartpy3) $ python
...
(ipartpy3) $ python -m unittest discover -s tests
... <full output copied below> 

the unit tests, the scripts and the notebooks all immediately error from various usage of from ipart.utils import funcs giving the ultimate exception of:

ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)

(although running import ipart in the Python session is successful and does not output that, or indeed any, error). I have included the full unit test run, and some representative script, outputs below if they may be helpful.

It seems an old location may be referenced for the import in question, or perhaps it is an attempted circular import. Whatever the cause, this issue needs resolving since this error is preventing any access to most of the functionality of IPART. Given the form it should be quick to fix.

Exact outputs

All with the same underlying error, but just to illustrate the extent to which it is emerging...

Some script outputs

$ cd scripts
$ python compute_ivt.py 
Traceback (most recent call last):
  File "compute_ivt.py", line 28, in <module>
    from ipart.utils import funcs
$ python trace_ARs.py 
Traceback (most recent call last):
  File "trace_ARs.py", line 65, in <module>
    from ipart.utils import plot
  File "/home/sadie/IPART/ipart/utils/plot.py", line 11, in <module>
    from . import funcs as functions
ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)

Full test output

Running python -m unittest discover -s tests under the root IPART repo directory returns:

EEEEE
======================================================================
ERROR: test_data (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_data
Traceback (most recent call last):
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/home/sadie/IPART/tests/test_data.py", line 15, in <module>
    from ipart.utils import funcs
ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)


======================================================================
ERROR: test_ipart (test_install.TestIPRT)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/sadie/IPART/tests/test_install.py", line 13, in test_ipart
    from ipart.thr import THR, rotatingTHR
ModuleNotFoundError: No module named 'ipart.thr'

======================================================================
ERROR: test_river_tracker1 (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_river_tracker1
Traceback (most recent call last):
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/home/sadie/IPART/tests/test_river_tracker1.py", line 14, in <module>
    from ipart.utils import funcs
ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)


======================================================================
ERROR: test_river_tracker2 (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_river_tracker2
Traceback (most recent call last):
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/home/sadie/IPART/tests/test_river_tracker2.py", line 13, in <module>
    from ipart.AR_tracer import readCSVRecord, trackARs, filterTracks
  File "/home/sadie/IPART/ipart/AR_tracer.py", line 19, in <module>
    from ipart.utils import funcs
ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)


======================================================================
ERROR: test_thr (unittest.loader._FailedTest)
----------------------------------------------------------------------
ImportError: Failed to import test module: test_thr
Traceback (most recent call last):
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 436, in _find_test_path
    module = self._get_module_from_name(name)
  File "/home/sadie/anaconda3/envs/ipartpy3/lib/python3.7/unittest/loader.py", line 377, in _get_module_from_name
    __import__(name)
  File "/home/sadie/IPART/tests/test_thr.py", line 14, in <module>
    from ipart.utils import funcs
ImportError: cannot import name 'funcs' from 'ipart.utils' (unknown location)


----------------------------------------------------------------------
Ran 5 tests in 0.001s

FAILED (errors=5)