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

add NIRISS imaging pipeline tutorial notebook #213

Closed
wants to merge 0 commits into from

Conversation

slamassa
Copy link
Contributor

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

  • [ X] The title of the notebook in a first-level heading (eg. <h1> or # in markdown).
  • [ X] A brief description of the notebook.
  • [ X] A table of contents in an ordered list (1., 2., etc. in Markdown).
  • The author(s) and affiliation(s) (if relevant).
  • [ X] 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

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

Text

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

Code

  • [X ] 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.
  • [ X] 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.
  • [ X] 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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@camipacifici
Copy link
Contributor

Thank you Steph!!
I'll do a quick science review and @haticekaratay or @gibsongreen will take care of the technical review.

@camipacifici
Copy link
Contributor

Hi @slamassa , thank you again for the notebook!
I see that there is no download for the files needed to run it. I took the liberty of fetching the files from MAST and putting them on Box here: https://stsci.box.com/s/8dreh2spez36pxgvx8isghem0nypsv5x
Can you please double check that these are indeed the files you intend to use in this notebook? For the future, do you have a preference between using this frozen version of the files or fetching them from MAST in the notebook? We can add the necessary code to do either.

@camipacifici
Copy link
Contributor

camipacifici commented Jan 31, 2024

Two more comments as part of the science review:

  • I would suggest to briefly explain why the background subtraction step and the resampling step are not run by default in stage 2. Should the user consider running them? What are the benefits?
  • I would suggest to provide a couple more examples of parameters that can be tuned in Image 3 since this is the most "science dependent" step. For example, what can be changed to get a more personalized source catalog? Or an image resampled differently than the default?

The rest looks great! The notebook can be marked as "Advanced" instead of "Baseline" since it uses JWST data, Jdaviz, and does not carry any developer note.

@slamassa
Copy link
Contributor Author

slamassa commented Feb 1, 2024

@camipacifici: Thanks for the careful scientific review of the notebook!
Regarding your first comment - yep, that Box link lists the data files the notebook should read in. I don't have a strong preference between using the frozen files or fetching them from MAST. I think using the frozen files could be OK since other notebooks illustrate the workflow for downloading files from the archive, and it might be quick to read in files from Box rather than downloading them from MAST. But if it's standard practice to fetch the files from MAST, I would be happy to make those updates to the notebook.

@slamassa
Copy link
Contributor Author

slamassa commented Feb 5, 2024

@camipacifici - how do I set up the notebook to read in the files from Box? Is it possible to grab all files in the Box directory at once, or do I need to specify the individual filenames in order to download the data?

@camipacifici
Copy link
Contributor

@slamassa I would store a zipped version on box and do something like this in the notebook:

boxlink = 'boxlink.zip' # Need the actual static link here
boxfile = Path('filename.zip')
urllib.request.urlretrieve(boxlink, boxfile)

zf = zipfile.ZipFile(boxfile, 'r')
zf.extractall()

I am taking the example from the MOS notebook.
Apologies for not storing already a zipped version myself.

@slamassa
Copy link
Contributor Author

slamassa commented Feb 5, 2024

Thanks @camipacifici! I've zipped the files locally but I don't have permission to add them to the box folder. Could you perhaps grant me permission?
Or, if it's easier for you to upload them, you can grab the zip file from here:
/ifs/jwst/wit/niriss/lamassa/pipeline_tutorial_ntbks/1475_f150w.zip

@camipacifici
Copy link
Contributor

camipacifici commented Feb 6, 2024

Done! The box link will be:
https://data.science.stsci.edu/redirect/JWST/jwst-data_analysis_tools/niriss_imaging/1475_f150w.zip
(I also granted you access for the next time, sorry I forgot before)

@gibsongreen
Copy link
Collaborator

@slamassa since this is on your main branch, just a heads up to pull the latest changes from your origin/fork before you start editing.

There is one error that I ran into when I attempted to execute the notebook. It is the first cell in Stage 1 Processing. Towards the end of the loop I receive an error message stating a .fits files Header is missing its END card. I checked the original fits files and subsequent ones created from the notebook did not see the origin of the error.

Let me know if I can provide any more information about the execution of this cell!

@slamassa
Copy link
Contributor Author

slamassa commented Feb 7, 2024

Thanks @gibsongreen! I made some edits to a local version of the notebook to address the science comments that @camipacifici left. As part of those edits, I also updated how the data files are fetched, downloaded and read in, so I can check what happens when I execute those cells and see whether or not I get the same error.

But... how do I work the git magic to make sure that your edits and my edits are combined?

@gibsongreen
Copy link
Collaborator

Thanks @gibsongreen! I made some edits to a local version of the notebook to address the science comments that @camipacifici left. As part of those edits, I also updated how the data files are fetched, downloaded and read in, so I can check what happens when I execute those cells and see whether or not I get the same error.

But... how do I work the git magic to make sure that your edits and my edits are combined?

You will want to fetch the changes I made and pull them into your local repository.

When you do this, since we were working on the same file, you will receive a message indicating conflicts. Open whatever IDE you use, and you will see all the differences between what I pushed and your recent edits.

Our individual changes shouldn’t overlap much. If they do, select the option that accepts all of your changes; that will work best. I can redo any of the styling changes to ensure PEP8 adherence.

After this, it is just a matter of adding, committing, and pushing the combined changes.

A second option, albeit a little taboo in version control, is for you to send me the notebook with your changes. I can review it, analyze the differences, and either manually resolve and merge the conflicts, pushing those changes afterward, or we can arrange a meeting where I guide you through the process. We may still need to address the notebook execution error. I had intended to send you a video demonstrating how to recreate the error I encountered.

Feel free to reach out and let me know what works best for you!

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

Successfully merging this pull request may close these issues.

3 participants