spacetelescope / jdat_notebooks

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

use public viz.data_labels over viz.app.data_collection #203

Closed kecnry closed 1 month ago

kecnry commented 6 months ago

This PR accompanies https://github.com/spacetelescope/jdaviz/pull/2626 and should only be considered if/when that is both merged and released (EDIT: anytime after jdaviz 3.9 is released). Note that the existing syntax (viz.app.data_collection) will continue to work and is not being deprecated, it just isn't intended to be considered as public API.

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 6 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

camipacifici commented 1 month ago

Sorry I didn't see this one. @gibsongreen, @haticekaratay, can we merge this?

gibsongreen commented 1 month ago

Sorry I didn't see this one. @gibsongreen, @haticekaratay, can we merge this?

It might be overkill but rerunning CI might be worth it. I double checked to make sure the ifu_optimal notebook will still work (since spectrum_subset1 is in a try-catch, is using this, and gets used again later on in the notebook). The later cell runs as it did before, so no issues there with the update. The other two are in essence prints, so it should be good to merge.

haticekaratay commented 1 month ago

Sorry I didn't see this one. @gibsongreen, @haticekaratay, can we merge this?

It might be overkill but rerunning CI might be worth it. I double checked to make sure the ifu_optimal notebook will still work (since spectrum_subset1 is in a try-catch, is using this, and gets used again later on in the notebook). The later cell runs as it did before, so no issues there with the update. The other two are in essence prints, so it should be good to merge.

Thank you, @gibsongreen, for investigating this. I've checked, and these notebooks run successfully in the main. So, I would only merge if we have a passing CI. Please feel free to rerun the action. I think you already know what's happening, as I haven't looked into that yet. So marking you as the reviewer. 🙏 😃