search

spacetelescope / dat_pyinthesky

Notebooks for "notebook-driven development" for the Data Analysis Tools efforts
https://dat-pyinthesky.readthedocs.io/en/latest/
8 stars 44 forks source link

Added blackbody and polynomial fitting within Jdaviz #160

Closed orifox closed 2 years ago

orifox commented 2 years ago

Added blackbody and polynomial fitting within Jdaviz

review-notebook-app[bot] commented 2 years ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

ojustino commented 2 years ago

Hi @orifox, I want to discuss a problem I see with the imports before getting into the review monologue.

aplpy needs to import Astropy's block_reduce. This notebook's requirements file mandates Astropy v4.3.1 at a minimum, which came out this summer. aplpy was last released in 2019 and Astropy has moved block_reduce to a different location since then. This discrepancy leads to an ImportError in the notebook.

The solutions I see are to drop aplpy or impose an Astropy version >=4.0 and <4.1. What do you propose?

ojustino commented 2 years ago

Hi @ofox,

Thank you for submitting these changes to your notebook. Please read on for your technical review.

Before you begin

The technical review helps ensure that contributed notebooks a) run from top to bottom, b) follow the PEP8 standards for Python code readability, and c) conform to the Institute's style guide for Jupyter Notebooks.

I've pushed the review as a new commit in this pull request. To view and edit the commit locally, follow these steps:

git checkout mrsupdate2
git fetch YOUR-REMOTE-FORK
git merge YOUR-REMOTE-FORK/mrsupdate2

_(YOUR-REMOTE-FORK is your fork's online copy. It's often origin, but if you don't know your name for it, run git remote -v and choose the one whose path ends with YOUR-USERNAME/dat_pyinthesky.git.)_

From here you can work on your branch as normal. If you have trouble with this step, please let me know before continuing.

Instructions

After updating your local copy of this branch, please open your notebook and address any warnings or errors you find.

If you see cells with output like this, it means some of your code doesn't follow the PEP8 standards of code readability:

image

(In the example above, INFO - 3:3: E111 means that the text entered on line 3 at index 3 caused the warning "E111". The violation is briefly described at the end of the message.)

You can test that your edits satisfy the standard by installing flake8 on the command line with:

pip install flake8==3.7.3 pycodestyle_magic

Then, restart the notebook and run the following cells:

image

After that, edit and re-run cells with warnings until you've fixed all of them. Please remember to delete the cells shown in the above image before pushing your changes back to this pull request.

If you have questions or feedback on specific cells, click the earlier message in this thread from the "review-notebook-app" bot. There, you can comment on specific cells and view what's changed in the new commit. I may also write comments there. Anything posted there will also be reflected in this pull request's conversational thread.

The three-point review (*action required*)

  1. ✅/⚠️ Notebook execution: I was able to run the notebook from top to bottom.

    • As mentioned in another review, I see inconsistent behavior in videos summoned with the HTML object. They played without issue in Firefox but had to be downloaded individually in Edge.

  2. ⚠️ Code style: There are a decent number of PEP8 violations.

    • Please follow the advice in the "Instructions" section above to fix them.

  3. ⚠️ Notebook style

    • There are several imports that happen after the "Introduction" section. I will mark a few, but please ensure that all imports happen at the beginning of the notebook.
    • Is there another format we use for developer notes? I can see them being confusing to an outside reader in their current state.
orifox commented 2 years ago

Ok, thank you @ojustino for walking me through this again. Your instructions were very clear and helpful. I think I got almost all the pep 8 errors removed. A couple things: 1) Still an error in Cell 1 regarding Syntax Error. 2) I am not going to worry about videos right now. It's ok if they download instead of play in cell. 3) Moved all the imports to the Introduction 4) We need to address developer note formats in the future.

If there are any small changes, please feel free to make them yourself. Let me know if any big changes are still needed.

ojustino commented 2 years ago

Hello @orifox, since we are deferring the video and developer note formatting discussions, these changes address everything else. You have successfully completed the technical review. I will do the merge, as we discussed.