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

NIRSpec IFU demos #140

Closed

Conversation

melanieclarke
Copy link

This PR includes three notebooks developed by the NIRSpec team demonstrating IFU data reduction. The intent is to transfer them to the JDAT team for long-term integration and support. They will require some editing to match the conventions and standards for notebooks in this repository.

This notebook checklist has been made available to us by the the Notebooks For All team.
Its purpose is to serve as a guide for both the notebook author and the technical reviewer highlighting critical aspects to consider when striving to develop an accessible and effective notebook.

The First Cell

  • The title of the notebook in a first-level heading (eg. <h1> or # in markdown).
  • A brief description of the notebook.
  • A table of contents in an ordered list (1., 2., etc. in Markdown).
  • The author(s) and affiliation(s) (if relevant).
  • The date first published.
  • The date last edited (if relevant).
  • A link to the notebook's source(s) (if relevant).

The Rest of the Cells

  • There is only one H1 (# in Markdown) used in the notebook.
  • The notebook uses other heading tags in order (meaning it does not skip numbers).

Text

  • All link text is descriptive. It tells users where they will be taken if they open the link.
  • All acronyms are defined at least the first time they are used.
  • Field-specific/specialized terms are used when needed, but not excessively.

Code

  • Code sections are introduced and explained before they appear in the notebook. This can be fulfilled with a heading in a prior Markdown cell, a sentence preceding it, or a code comment in the code section.
  • Code has explanatory comments (if relevant). This is most important for long sections of code.
  • If the author has control over the syntax highlighting theme in the notebook, that theme has enough color contrast to be legible.
  • Code and code explanations focus on one task at a time. Unless comparison is the point of the notebook, only one method for completing the task is described at a time.

Images

  • All images (jpg, png, svgs) have an image description. This could be

    • Alt text (an alt property)
    • Empty alt text for decorative images/images meant to be skipped (an alt attribute with no value)
    • Captions
    • If no other options will work, the image is decribed in surrounding paragraphs.
  • Any text present in images exists in a text form outside of the image (this can be alt text, captions, or surrounding text.)

Visualizations

  • All visualizations have an image description. Review the previous section, Images, for more information on how to add it.

  • Visualization descriptions include

    • The type of visualization (like bar chart, scatter plot, etc.)
    • Title
    • Axis labels and range
    • Key or legend
    • An explanation of the visualization's significance to the notebook (like the trend, an outlier in the data, what the author learned from it, etc.)
  • All visualizations and their parts have enough color contrast (color contrast checker) to be legible. Remember that transparent colors have lower contrast than their opaque versions.

  • All visualizations convey information with more visual cues than color coding. Use text labels, patterns, or icons alongside color to achieve this.

  • All visualizations have an additional way for notebook readers to access the information. Linking to the original data, including a table of the data in the same notebook, or sonifying the plot are all options.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@melanieclarke melanieclarke temporarily deployed to ci_env August 28, 2023 14:26 — with GitHub Actions Inactive
@melanieclarke melanieclarke temporarily deployed to ci_env August 28, 2023 14:26 — with GitHub Actions Inactive
@melanieclarke melanieclarke temporarily deployed to ci_env August 28, 2023 14:26 — with GitHub Actions Inactive
@haticekaratay haticekaratay added the Technical Review Add this for automated PEP8 checks of notebooks label Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

Hello @melanieclarke,

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

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 nirspec_ifu_demo
git fetch YOUR-REMOTE-FORK
git merge YOUR-REMOTE-FORK/nirspec_ifu_demo

(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 melanieclarke/jdat_notebooks.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.9.2 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.

@haticekaratay
Copy link
Collaborator

@melanieclarke,
We have recently updated the CI for the PEP8 checks. Can you please rebase your branch with the main? Then I will continue with my review. Thanks!

@haticekaratay
Copy link
Collaborator

Hello @melanieclarke, I will help fix style errors to ensure the notebooks merge with the main. In the meantime, I think it would be a good idea to address the execution errors. You can find the errors in the logs provided. https://github.com/spacetelescope/jdat_notebooks/actions/runs/6398259254/job/17368005414?pr=140

@melanieclarke
Copy link
Author

@haticekaratay - Sorry, I'm not the original author of these notebooks, and I don't have time allocated to updating them at the moment. I'm just passing them on, as requested. If the necessary edits can't be done by the JDAT team, let me know and I'll pass the request for support back to the NIRSpec team.

@haticekaratay
Copy link
Collaborator

Style errors have been resolved for the three notebooks, but we have a new issue. Downloading the files caused a significant depletion of disk space on our CI runners. I request that the authors reevaluate if they can reduce the number of downloaded files that may not be necessary for the workflow. Thanks!

@camipacifici
Copy link
Contributor

@melanieclarke, revisiting this notebook. unfortunately it is necessary to reduce the amount of data this notebook uses. can you or somebody else on the NIRSpec team please get in touch with us in DATB so we can agree on a path forward? thank you!

@melanieclarke
Copy link
Author

@camipacifici - Kayli Glidic, the original author, was planning to look into that but hasn't had time. I'll ping her again and let her know DATB is ready to revisit this notebook.

@melanieclarke
Copy link
Author

@kglidic says she'll look into it this week.

@camipacifici
Copy link
Contributor

Thank you!

@kglidic
Copy link
Contributor

kglidic commented Feb 28, 2024

Hi, I’m currently working on reducing the amount of data that is downloaded in these notebooks. Initially, I was downloading numerous intermediate products that weren't being utilized, so I'm now in the process of filtering through these. The essential files needed for these notebooks to function properly are the uncal.fits files (in addition to a handful of products from MAST which we use to do comparisons). In the notebook utilizing data from PID 2732, there are 8 uncal.fits files (each approximately 125 MB). For the notebook involving data from PID 2729, there are 16 uncal.fits files to be downloaded (for NRS1 & NRS2, approximately 62.5 MB each). Would this quantity of data still be excessive for downloading and running these notebooks?

If this is still too many files, I can minimize the number of files to be downloaded by exclusively saving the computed products utilized for the plots in the notebook by providing a Box link. Actually, I’d like to provide a Box link in these notebooks for at least downloading the older processed products from MAST (from the time that I made this notebook). It would be a handful of files that would get downloaded from the link. We use these products to do comparisons in the notebook. However, these products may have been re-processed in MAST since I made this notebook and if users download newer products from MAST the commentary throughout the notebook may no longer be valid/confusing. Is this an option? Would I be able to get access to the Box folder where I can upload a zip file for these products?

@kglidic
Copy link
Contributor

kglidic commented Mar 6, 2024

Hi @camipacifici , realized it was probably best to actually tag someone in my previous comment. I went through the data in these notebooks and reduced the amount of data that is getting downloaded. Here is the breakdown:

  • ero_nirspec_ifu_02732_demo.ipynb: 1.2 G downloaded (previously it was 4.6 G)
  • ero_nirspec_ifu_02732_demo_pointsource.ipynb: 1.0 G downloaded (previously it was 4.6 G)
  • ero_nirspec_ifu_02729_demo.ipynb: 2.7 G downloaded (previously it was 18 G)

Will these reduced amounts work? I also would like to have a handful of this data come from a Box link (for the older products in MAST that I was working with when I made these notebooks). If you download the products from MAST now, they have been re-processed and it would make the comments throughout the notebook confusing. Can this be done? I have the notebooks on my end set up and ready to downloaded data from a Box link, I would just need to get access to correct location in Box. I've also gone through the notebooks to fix any additional style errors, so if everything sounds doable, I can pass these notebooks along.

@camipacifici
Copy link
Contributor

Hi @kglidic, thank you so much for getting back to this! I just sent you an invite to the box folder where you can store the data. Feel free to create a folder in there. I can show you how to build the link with the human readable redirect once your zip file il there (you can also look at other notebooks for examples).
@mgough-970, can you check the message above and tell us if these data sizes are acceptable?
Thank you!

@melanieclarke
Copy link
Author

I'm closing this PR from my fork - @kglidic will submit a new one momentarily from her own fork and we can go from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Technical Review Add this for automated PEP8 checks of notebooks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants