spacetelescope / jwst-pipeline-notebooks

JWST Calibration Pipeline Notebooks
https://spacetelescope.github.io/jwst-pipeline-notebooks/
Other
2 stars 14 forks source link

NIRISS SOSS calibrating TSO products and light curve generating #6

Open tbainesUA opened 5 months ago

tbainesUA 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

tbainesUA commented 5 months ago

I've gone through my notebook and attempted to correct them where some tests were failing. I've fixed the PEP8 standard issues. However, I am unsure about the Notebook execution steps. For the second notebook 02_niriss_soss_spec2_generate_lightcurves, it uses the saved output results that are produced from the first notebook 01_niriss_soss_detector1_reduction.

In regard to environment setup to run notebook I had to modify the existing requirement.txt for the tests to run. I addition I've provided a soss specific requirements as well if it needed at all.

gibsongreen commented 4 months ago

I've gone through my notebook and attempted to correct them where some tests were failing. I've fixed the PEP8 standard issues. However, I am unsure about the Notebook execution steps. For the second notebook 02_niriss_soss_spec2_generate_lightcurves, it uses the saved output results that are produced from the first notebook 01_niriss_soss_detector1_reduction.

In regard to environment setup to run notebook I had to modify the existing requirement.txt for the tests to run. I addition I've provided a soss specific requirements as well if it needed at all.

Hello Tyler,

After running through the two notebooks, there are an only a few technical changes necessary to pass CI.

The main change that we will have to make to the notebooks is storing the generated data from the first notebook into a Box directory, and adding some code so this Box directory and the generated data is accessible to the second notebook.

For some background, the CI does not look at the PR in totality, but rather, it runs the notebooks separately. Thus, the second notebook will not have access to the data generated by the first notebook. By adding a Box directory, this will ensure that both notebooks can be run individually (by both a user and the CI).

gibsongreen commented 4 months ago

A quick update: We had our biweekly CI meeting this afternoon and came to a resolution for this notebook and how to implement PRs with multiple notebooks that need to be run in order for any future PR's.

For this notebook, I will work to add the Box link with the generated data for the first notebook. There will be an additional comment added at this section explaining the reasoning behind the code block that will enable to second notebook to pass CI with said Box link. This will be a temporary solution while the CI team works on the solution to enable specific notebooks to run in order.

Once this is implemented and tested by the CI team, we will create a second PR for this notebook, striping the extract code block and comment that allowed the CI to originally pass CI.

drlaw1558 commented 4 months ago

Just adding a quick note that the jwst-pipeline-notebooks are intended to be rather different from the previous JDAT notebooks, and should not in general split a given observing mode between two different notebooks. As such, there should be no need for notebooks to run in order. It would be best to pause further testing here until we've got a template notebook finalized and can discuss content and overall design prior to digging into more technical details.