spacetelescope / jdat_notebooks

JWST Data Analysis Tools Notebooks
https://spacetelescope.github.io/jdat_notebooks/
98 stars 80 forks source link

add NIRISS imaging pipeline tutorial notebook #213

Closed slamassa closed 4 months ago

slamassa commented 5 months ago

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 Rest of the Cells

Text

Code

Images

Visualizations

review-notebook-app[bot] commented 5 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

camipacifici commented 5 months ago

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

camipacifici commented 5 months ago

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 commented 5 months ago

Two more comments as part of the science review:

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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

@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 commented 4 months ago

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 commented 4 months ago

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!