JOSS Review Comments

[nkrusch]

Thanks for the interesting library, I enjoyed exploring it. I was able to complete the JOSS checklist and complete each item satisfactorily.

I will make a few suggestions for enhancements, beyond the checklist.

Software paper

• [x] Selected use cases: Estimating discharge (pg. 3) resembles example examples/sim_battery_eol.py? If this correct, suggest adding a link to that code example in the paper (similar to model link on pg. 2).

• [x] References: add the 2 missing DOIs (cf. bot issue comment).

• [x] Editorial: the last commit to paper introduced a few typos:

  • L25: softwares => software packages
  • L26: limted => limited
  • L34: pytourch => PyTorch

Functionality

For functionality checks, I installed the software, completed tutorial.ipynb, and those worked as expected. The repo organization is clear, and I have no major comments.

Relating to suggestions in issue #506, I also ran the unit tests, following setup steps and commands from "python package" workflow:

python -m pip install --upgrade pip
python -m pip install -e .
python -m pip install notebook
python -m pip install testbook
python -m pip install requests
python -m tests

I ran these tests first on apple M1 mac, Python 3.10.5, and it terminated on this error:

======================================================================
ERROR: test_lstm_simple (tests.test_data_model.TestDataModel)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "prog_models/tests/test_data_model.py", line 148, in test_lstm_simple
    m3 = deepcopy(m2)
UnboundLocalError: local variable 'm2' referenced before assignment
----------------------------------------------------------------------

This may relate to the architecture (M1 has issues with Tensorflow/Keras, in my experience). I repeated the steps using Ubuntu 18.04, Python 3.8; then all tests completed to end. I mention this because I also recommend clarification on these tests.

• [x] Suggested that the project docs would specify:

1. How to set up a test env
2. OS/achitecture requirements
3. Commands to run unit tests

[teubert]

Thank you @nkrusch for your comments. I've taken all of your suggestions. The changes to the paper can be found in here: https://github.com/nasa/prog_models/commit/bccff282fb7b58ebf937809a1f212be3f33b42f6 and I added some instructions for running unit tests here: https://github.com/nasa/progpy/commit/820fcd627b9acc404a3e98eaee67bde25953d11c

Re: your issues with the tests. Tensorflow has been a real headache for me! I'm actually also on an M1 mac and am not running into the described issue, but some other M1 mac users have described similar behavior. I've found it challenging to identify exactly what configuration will lead to that issue.

Let me know if you think my changes resolve your review comments.

[nkrusch]

All tasks are resolved, I will close this.