MRS Science Review Update

[orifox]

Science Review new PR for Beth's original PR147.

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ibusko]

Technical review of notebook #1 (JWST_Mstar_dataAnalysis_runpipeline.ipynb)

Running the notebook

• The notebook runs to completion

• Any reason for not having jwst as a dependency in requirements.txt?

Input data

• Are properly stored in Box using redirect.

Libraries

• Currently available core-libraries functionalities and visualization tools are used.

PEP-8 compliance:

• Running 'flake8' on the notebook cells results in this report:

  Cell 1: E225 missing whitespace around operator E231 missing whitespace after ':' E231 missing whitespace after ',' Cell 5 line 9,15,21: E231 missing whitespace after ',' Cell 6 line 1: E265 block comment should start with '# ' Cell 7 line 3,4,11,14: E225 missing whitespace around operator Cell 7 line 15,26,29,30: E231 missing whitespace after ',' Cell 8 line 3: E265 block comment should start with '# ' Cell 8 line 10,27: E231 missing whitespace after ',' Cell 9 line 7,10,27,33,35,36,40,43: E231 missing whitespace after ',' Cell 9 line 25,27,39,40,43,46,47: E225 missing whitespace around operator Cell 9 line 33: E262 inline comment should start with '# ' Cell 10 line 6,14: E231 missing whitespace after ',' Cell 10 line 3,4,7,11,12,13: E225 missing whitespace around operator Cell 11 line 5: E231 missing whitespace after ',' Cell 11 line 7: E225 missing whitespace around operator Cell 12 line 9,10: E225 missing whitespace around operator

Build

• There are conflicting files.

Developer Notes

• None

Comments

• The initial introductory section is confusing. It seems to be refering to tasks that are executed in the 2nd notebook, not in this one. It refers to the use of spectral_cube, but no instance of that package appears in the code. The ToDo section seems to be applicable to notebook 2 only.

[ibusko]

Technical review of notebook #2 (JWST_Mstar_dataAnalysis_analysis.ipynb)

Running the notebook

• The notebook runs to completion

Input data

• Are properly stored in Box using redirect, or are generated by notebook 1.

Libraries

• Currently available core-libraries functionalities and visualization tools are used.

PEP-8 compliance:

• Running 'flake8' on the notebook cells results in this report:

  Cell 1 line 9: E225 missing whitespace around operator Cell 1 line 9,10,11: E231 missing whitespace after ':' Cell 1 line 9,10,11: E231 missing whitespace after ',' Cell 4 line 9: E231 missing whitespace after ',' Cell 5 line 1: E265 block comment should start with '# ' Cell 8 line 6,9,12,13,14,25,27: E251 unexpected spaces around keyword / parameter equal Cell 8 line 12,17,25: E231 missing whitespace after ',' Cell 8 line 23: E225 missing whitespace around operator Cell 8 line 19: E265 block comment should start with '# ' Cell 8 line 20: E402 module level import not at top of file Cell 20 line 2: E231 missing whitespace after ',' Cell 21 line 5: E305 expected 2 blank lines after class or function definition, found 1 Cell 21 line 15,16,25: E211 whitespace before '(' Cell 22 line 2,23,10,15,16: E231 missing whitespace after ',' Cell 22 line 2,3,4,8,15,16,17,21: E251 unexpected spaces around keyword / parameter equals

Build

• There are conflicting files.

Developer Notes

• Some developer notes are already addressed (they include a pointer to the corresponding Jira ticket).

• "Add filter to SpectrumList.read()" - this already exists. Use the extension='fits' parameter in the read() method.

• "Fix the uncertainties" - shouldn't this be fixed in the first notebook? Or perhaps in the tools used by it to create the files. The files with zeroed uncertainties are created in the first notebook step.

• Fitting to Phoenix models - added a comment to the existing, related Jira ticket https://jira.stsci.edu/browse/JDAT-1793

• The To Do paragraph is a bit misleading. It sounds like reminders for scientific development, but two tasks that refer to "making a function" sound more like developer notes.

Comments

• The initial introductory text refers to the use of spectral_cube, but no instance of that package appears in the code.

[ibusko]

Post-technical review:

After the points raised in the technical review were addressed, I did a second pass of PEP-8 compliance checking, absence of conflicts, and verified that both notebooks run on a pristine environment, with no issues .