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

Review from @ashleefv #53

Open
13 of 34 tasks
devarops opened this issue Jun 7, 2024 · 0 comments
Open
13 of 34 tasks

Review from @ashleefv #53

devarops opened this issue Jun 7, 2024 · 0 comments

Comments

@devarops
Copy link
Member

devarops commented Jun 7, 2024

paper.md

  • typo: Figure 2 caption density units should be kg/m^2 instead of kg/ha to match the y-axis units
  • Figure 3 caption also lists density as kg/ha. Is it more useful to stay in density kg/ha or kg/m^2 throughout the paper.md and the code? Consistency is desireable.
  • typo: "resorting" should be "restoring" in Introduction.
  • typo: Figure 1 caption "even" should be "event" and "line" should be "curve"
  • paper.md is inside the examples subfolder. It seems weird to include the code in the paper.md file.

We have fixed the typos.
We have removed the code from the paper.md file.

readme.md

  • the link to where to explore the Jupyter notebooks does not work. The local host typically needs to be deployed by Jupyter on one's local machine.

examples/calibration-demo.ipynb

  • The text in the first section includes TeX commands for \eqref and Figure \ref cross-referencing that are not understood in the Jupyter notebook. They both render as ??? in the notebook.
  • /workdir is not automatically set.
  • In [3]: I needed to change the file location to "../tests/data/flow.csv" for the csv file to be read.
  • In [5]: "figures/calibration.png" works as the path. The paper.md file shows the following for this line:
  • plt.savefig("examples/figures/calibration.png", dpi=300, transparent=True)
  • but that disagrees with the calibration.demo.ipynb code and doesn't run on my computer either.
  • In [6]: "../tests/data/profile.csv"
  • In [8]: "figures/plots.png"
  • In [12]: "figures/density_profile.png"
  • In [15]: "figures/contour_plot.png"
  • The text after the figure saved as calibration.png is the caption for Figure 1 in the paper.md file. That Markdown text does not render correctly in the .ipynb file.
  • The text after the figure saved as plots.png is in Spanish "Aca va el pie de figura" or "Here goes the figure caption" and does not render correctly in the .ipynb file.
  • The text after the figure saved as density_profile.png is in Spanish "Aca va el pie de figura" or "Here goes the figure caption" and does not render correctly in the .ipynb file.
  • The text after the figure saved as contour_plot.png is the caption for Figure 3 in the paper.md file. That Markdown text does not render correctly in the .ipynb file.
  • The text in the last section includes TeX commands Figure \ref cross-referencing that is not understood in the Jupyter notebook and renders as ??? in the notebook. Recommended to move this text to the top of the notebook.

examples/proceedings.ipynb

  • This seems like a draft of paper.md and may not need to be in the repository anymore. I did not review this file or the preprint.md file as they are neither the paper.md or the main code or examples.

We have removed proceedings.ipynb and preprint.md.

examples/tiling_demo.ipynb

  • This file does not run for me because there are many nested instances of /workdir/ in the .ipynb file as well as the .json file. workdir needs to be specified or automated for this to run. I replaced /workdir/ with ../ everywhere in the .ipynb and .json files to get this to run.
  • The text before the Discussion section is in Spanish "Aca va el pie de figura" or "Here goes the figure caption" and does not render correctly in the .ipynb file.
  • The Discussion at the end of this notebook could be easily included as part of the introduction. Then Discussion and References sections would not be included. Currently References section is empty in this .ipynb file and is not needed.

Throughout the nerd repository

  • I recommend replacing relative paths (dot notation) of imports
from . import 
from .. import

with the specific absolute paths, e.g., in nerd\calibration\best_density_function.py

from nerd.calibration import get_rmse_from_function_array
from nerd import density_functions as df

PEP 8 standard explicitly recommends absolute imports: https://realpython.com/absolute-vs-relative-python-imports/
This recommendation applies to the following scripts:

  • nerd/init.py
  • nerd/mapping/init.py
  • nerd/io/init.py
  • nerd/density_functions/init.py
  • nerd/calibration/init.py
  • nerd/calibration/model.py
  • nerd/calibration/best_density_function.py
  • nerd/calibration/rmse.py

Your files nerd/io/import_data.py and nerd/mapping/tiling.py already use the recommended absolute paths for imports.

We have replaced relative paths (dot notation) of imports with with the specific absolute paths.


This issue is part of a JOSS review:

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

No branches or pull requests

1 participant