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 Demo Notebook (NEW TEMPLATE) #29

Merged
merged 17 commits into from
Jan 31, 2025
Merged

Conversation

kglidic
Copy link
Contributor

@kglidic kglidic commented Nov 19, 2024

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.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rizeladiaz
Copy link
Collaborator

Pushed reviewed notebook with following changes:

  1. Changed name to meet standard for jwst-pipeline-notebooks
  2. Created ifu subdirectory and moved files there
  3. Removed requests>=2.32.3 from requirements file
  4. Removed CRDS from the requirements as it is downloaded with jwst
  5. Changed JWST pipeline version and CRDS context section in the introductory cell to hvae the default text
  6. Changed TOC to have a new section to display the products. Section 8. The modifying the extract 1D moved to Section 9.
  7. Changed the configuration section to add a subsection on how to install the dependencies
  8. Allowed logs to be displayed. Users need to get used to check the logs and identify any problems.
  9. Changed user basedir to agree with the proposal and observation of this notebook
  10. Removed mention of comparing products with other MAST products because that is out of the scope of this notebook.
  11. Added section for setting processing steps that should execute. This in the configuration cell.
  12. Added the standard text for the section “Set CRDS context and paths” in the cell and commented the row setting up the context as now it should be automatically set by the pipeline.
  13. Removed the print of the CRDS_CONTEXT from this cell because if it is not set here it will error.
  14. From imports: removed “import request” because this is not used after removing the cell where the data is requested outside the pipeline run.
  15. Moved the astroquery import to its own section
  16. Added “import crds” this is needed to import the full CRDS updates and also pull the correct context.
  17. Added a print context to this cell because after importing CRDS it can identify the context for that pipeline version.
  18. Added explanation to what the function to update the association files does.
  19. Added cell to track runtime
  20. Removed extra line and spaces
  21. Removed sections where related to comparison of current data with MAST
  22. Added description to some of the cells.
  23. Removed space for commented commands to differentiate from real comments.
  24. Clarified language in use of param reference files.
  25. Moved visualization to end
  26. Removed get_jwst_file. Not used
  27. Added logic to skip processing background in files do not exist

@rizeladiaz
Copy link
Collaborator

I just pushed the last changes to this notebook. It fixes a couple of problems and adds a way to create an stahe3 association that includes background observations . However, need to find a dataset to test this further. MAST is down so I was unable to get data to do this. Also, pytest keeps timingout in my computer. Increased value but not working. Note that the pytest worked before. Not sure if MAST issues are affecting it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the changes to this file.

notebooks/NIRSPEC/ifu/JWPipeNB-NIRSpec-IFU.ipynb Outdated Show resolved Hide resolved
@kglidic
Copy link
Contributor Author

kglidic commented Jan 14, 2025

I have completed my review of the changes made to the notebook. I tested it using both the demo data and a non-demo dataset (PID 1309, Observation 13) that included a dedicated background exposure (Observation 14). In the non-demo dataset, both the science and background exposures contained leakcal members so I made sure the notebook could handle those cases. During my check, I identified a few areas in the notebook that required minor fixes or updates. I have already addressed these in this update and have summarized the changes I made:

The addition of creating an association for Spec3 works well with the notebook set up. There were a few things that I changed for that part of the code. The ASN file created looks as expected after these edits:

  • I changed where this file gets saved. I have saved it to the association directory.
  • When creating the spec3 associations, I had to modify the code to change the exptype of the background files; otherwise, they get labeled as science.
  • Made spec3_asn a list when creating the association in the notebook.

While testing the non-demo IFU data, I realized it did not account for background observations with leakcal exposures, which require an ASN file as input for spec2. The updates below address these issues.

  • Modified the ASN file filtering logic when downloading from MAST to ensure candidate ASN files, which may include associations with imprint and background exposures, are always downloaded. Background observation ASN files are now also downloaded, as they can include imprint (leakcal) exposures with IFU data.
  • Added a function to identify if any leakcal exposures are present in an association file.
  • Updated the cell that categorizes the spec2 association files to include filtering between science and background associations. The code now prioritizes background ASN files with imprint exposures, if available. If no background ASN files are provided, it will default back to processing the rate files directly if doing master background subtraction.

Other edits:

  • Fixed known issue link in the introduction.
  • In the notebook and the TOC, I added subsections that break up calibrating the science and background in Stages 1/2. 
  • Fixed the titles and the linkage in the TOC to the spec2 and spec3 association sections.
  • Removed any remaining parts in the notebook linked to the compare_mast ability in the previous version of this notebook.
  • As a check, I added two print lines in the import section to print out the default context for the build and the one currently set.
  • I copied the security note about the MAST token from the FS notebook reviews to this one.
  • I removed redundant timestamps in the notebook. I also have the time reported in minutes vs. seconds.
  • I copied the information about the types of spec2 association files from other notebook reviews here.
  • Added clean_flicker_noise to the det1dict.
  • In det1dict, I separated the code into a few cells to associate the blue comments more easily with the referenced step.
  • Fixed logging in the update_asn_paths function.
  • Fixed background subtraction methods link.
  • Added try-except blocks to the cells running Spec2. This was necessary because if an NRS2 file in the rate file list has no slices that fall on it (like with PRISM data), the process would crash.
  • Had to make separate lists for the rate and rateints files otherwise when processing background observations the rateints would cause a crash.
  • Added a warning that associations for moving targets with background exposures will cause spec3 to crash in build 11.1. I found this out after trying to test a moving target dataset. This warning can be removed when the notebook is updated in the next build.

There may be some PEP8 style issues to fix after this update.

@rizeladiaz
Copy link
Collaborator

I fixed the remaining PEP8 issues. The last PRP8 failures are due to the way we format the notebooks. However, there is a case where it complains about the indentation in a cell. It is correct but still complains about it. Maybe someone can figure out why?

I will be doing more testing but this version can be published as is.

@drlaw1558
Copy link
Collaborator

drlaw1558 commented Jan 20, 2025

A few comments:

  • As with the fixed slit notebooks, the download command isn't working properly. It should be, e.g.,
    sci_manifest = Observations.download_file(file, local_path=os.path.join(uncal_dir, Path(file).name))
    EDIT: I see, this is something that was enabled with astroquery 0.4.8 released just last week. No need to change, but let's not rely on pre-release dependencies.

  • Please move the directory to IFU/ rather than ifu/, and remove the extra .py file

  • I think we should revise how associations are handled by this notebook; at the moment it downloads asns for test data, but does nothing for other programs. Presumably it relies on users to download them too, but for the IFU it should be simple to create them from scratch as part of the notebook. See, e.g., the MRS notebook which creates asns on the fly. We should do that here too (along with the option of adding 'selfcal' exposures to the spec2 asn list), both for the demo data and for ordinary data. If you'd like I can take a crack at doing this.

  • The potential wrinkle to the above: how are leakcal observations handled? Can the imprint exposure that's meant to go with a particular science (or background) exposure be identified as such from the header information? If not, we should change that in DMS.

@rizeladiaz
Copy link
Collaborator

Hi David, I added some code to build the association for the general case and Kayli added the leakcal. Let me check this notebook again and try to figure out if we need to make things more clear.

@rizeladiaz
Copy link
Collaborator

Updated notebook to address David's requests above. Changed notebook to use jwst 1.17.1 and removed unnecessary dependencies in the requirement files (as mentioned in other comments by David and Megan).

Added code to make a copy of the extractid reference file when trying to write the modified version to a directory with no write permission. Fixed on PEP8 issue.

@drlaw1558
Copy link
Collaborator

@rizeladiaz I think I see the issue- we're talking past each other a bit. There's a method in the notebook for creating spec3 asns, but not for creating spec2 asns. In this case (given the lengthy iteration and multiple versions already) I think the simplest approach would be to merge this notebook as-is since its already functional, and then we can look into any further updates.

Copy link
Collaborator

@drlaw1558 drlaw1558 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG2M; it's probably useful to make a few further changes to association handling for general utility, but that can be handled as a delta against this latest version.

@drlaw1558 drlaw1558 merged commit 6657ff0 into spacetelescope:main Jan 31, 2025
2 checks passed
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