marcoalopez / grainsizetools Goto Github PK

This is related to the installation (issue #7), and should be addressed before then. For python packaging, it is standard install the module as a single module and import packages from there (rather than individually installing each of the python scripts). e.g.

import grain_size_tools as gst
gst.extract_column()

This way there is less chance of name clash, and it is pretty clear what you are importing when.

I will start a pull request to give a few pointers

The text in the License is the entire, all that needs to be included is the portion about the application of the license (see: https://opensource.org/licenses/Apache-2.0):

   Copyright [yyyy] [name of copyright owner]

   Licensed under the Apache License, Version 2.0 (the "License");
   you may not use this file except in compliance with the License.
   You may obtain a copy of the License at

     http://www.apache.org/licenses/LICENSE-2.0

   Unless required by applicable law or agreed to in writing, software
   distributed under the License is distributed on an "AS IS" BASIS,
   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
   See the License for the specific language governing permissions and
   limitations under the License.

where the [yyyy] is replaced by the year and [name of copyright owner] is replaced with the name of the copyright owners. Make sure you don't leave the brackets.

Is there a doi that can be added to the Twiss and Moores reference in the JOSS paper?

The docs direct new users to a function called calc_diameters which does not appear to exist:

NameError: name 'calc_diameters' is not defined

This issue is related to the ongoing JOSS review of this package. One of the criteria is:

Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

However, I found myself struggling to easily find the dependency list to get started. What do you think about adding a requirements.txt or environment.yml file?

This is related to the JOSS pre-review issue: openjournals/joss-reviews#811

The package layout does not follow standard python packaging practices (see for example: https://packaging.python.org/tutorials/packaging-projects/).

/grain_size_tools  # repo
   /grain_size_tools  # python module
       __init__.py  # contains import to be brought in when the user runs import grain_size_tools
       tools.py
       piezometers.py
       ...

This would allow more flexible use - for example if a user wants to install your package through pypi and import it into a Jupyter notebook as a part of their workflow.

The main items to be addressed are:

• folder structure and inclusion of __init__.py files (this allows from grain_size_tools import piezometers for example) - a good resource on why these are used is here: https://docs.python.org/3/tutorial/modules.html#packages
• setup.py for installing (this way the user can run python setup.py install from the repository-level directory to install the package - https://packaging.python.org/tutorials/packaging-projects/
• (optional): you can deploy your package to pypi so that instead of someone having to download the GitHub repository and running the setup.py, they can run pip install grain_size_tools from a command line and it performs the download and install: https://packaging.python.org/tutorials/packaging-projects/

Please let me know if there are more resources I can send along that would be helpful!

I don't see a clear indication of automated tests or manual steps described so that the function of the software can be verified. Are there manual tests that can be run to check that the software is performing as expected?

For example, if there are simple examples where you know what the output of a function should be, then you can test that. The pytest module is a good resource to check out for setting up testing: https://docs.pytest.org/en/latest/

In addition, you can use a continuous integration service that will run the tests every time you update the code (or if a contributor suggests an update). This is a really helpful thing to do if you would like to invite others to participate in the development and maintenance of the code base. For example using TravisCI, there are GitHub instructions here: https://help.github.com/enterprise/2.14/admin/guides/developer-workflow/continuous-integration-using-travis-ci/

Right now, the paths in the Jupyter notebooks are specific to your machine, it would be preferable to use relative paths so that when a user downloads them, they run out-of-the-box. e.g.

%run C:/Users/Marco/Documents/GitHub/GrainSizeTools/grain_size_tools/GrainSizeTools_script.py

could instead be

%run ../grain_size_tools/GrainSizeTools_script.py

and similarly for the files included in the notebook.

'../data_set.txt'

B parameter in Van der Wal olivine piezometer must be 1355.4 instead of 4250

In the README and the docs, it is really helpful for useability to have a section on installation. Since the requirements are quite simple here, the installation instructions should be pretty straightforwad

• git clone the repository
• pip install requirements
• test an example

(see for example: https://github.com/scikit-learn/scikit-learn#installation, https://github.com/seequent/properties#installation)

line 143 in tools.py is

gsd = np.std(np.log(diameters))  # multiplicative (geometric) standard deviation

and should be

gsd = np.exp(np.std(np.log(diameters)))

dataset['diameters'] = 2 * np.sqrt(dataset['Area'] / np.pi)
Traceback (most recent call last):

File "C:\Users\HP\anaconda3\lib\site-packages\pandas\core\indexes\base.py", line 2895, in get_loc
return self._engine.get_loc(casted_key)

File "pandas_libs\index.pyx", line 70, in pandas._libs.index.IndexEngine.get_loc

File "pandas_libs\index.pyx", line 101, in pandas._libs.index.IndexEngine.get_loc

File "pandas_libs\hashtable_class_helper.pxi", line 1675, in pandas._libs.hashtable.PyObjectHashTable.get_item

File "pandas_libs\hashtable_class_helper.pxi", line 1683, in pandas._libs.hashtable.PyObjectHashTable.get_item

KeyError: 'Area'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):

File "", line 1, in
dataset['diameters'] = 2 * np.sqrt(dataset['Area'] / np.pi)

File "C:\Users\HP\anaconda3\lib\site-packages\pandas\core\frame.py", line 2902, in getitem
indexer = self.columns.get_loc(key)

File "C:\Users\HP\anaconda3\lib\site-packages\pandas\core\indexes\base.py", line 2897, in get_loc
raise KeyError(key) from err

KeyError: 'Area'