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

DM-18530 Add Firefly verification notebooks to lsst-sqre notebook-demo repository #18

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

stargaser
Copy link
Contributor

@stargaser stargaser commented Mar 29, 2019

For testing the 17.0.1rc1 and 17.0.1 versions of the LSST Science Platform, these notebooks have been collected. The intent is to run these on release candidates to verify that Firefly features are still working. They will also be run when testing aspects of the Science Platform like authorization tokens.

I named the subdirectory "firefly_features" since these are features that a user should feel confident using with the named release.

  • added README explaining what these notebooks are for
  • updated intro-with-globular notebook to work with the 17.0.1 18.1.0 release
  • updated the Firefly.ipynb notebook that is in the parent directory, mainly to keep these notebooks all together
  • updated HSC Footprints notebook that is in display_firefly
  • added a new notebook that tests much, but not all, of the module documentation

EDIT: removed the duplicate Firefly.ipynb notebook and the HSC Footprints notebook.

@stargaser
Copy link
Contributor Author

@SimonKrughoff Would you please assign yourself as the reviewer?

cc @gpdf

@SimonKrughoff SimonKrughoff self-requested a review March 29, 2019 18:32
Copy link
Contributor

@gpdf gpdf left a comment

Choose a reason for hiding this comment

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

Is the intent that firefly_features/Firefly.ipynb ultimately replace the existing Firefly.ipynb one level up?

@stargaser
Copy link
Contributor Author

Good question -- the Firefly.ipynb notebooks are duplicates right now, except for one Markdown cell I added with version information. I was planning to leave the disposition of the top-level Firefly notebook to @SimonKrughoff.

Copy link
Contributor

@SimonKrughoff SimonKrughoff left a comment

Choose a reason for hiding this comment

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

  • Is the proposal to make these notebooks available to all users, i.e. in prod or just to people who know to check out master?
  • I don't think I want to have two notebooks with the same name: e.g. two Firefly.ipynbs.
  • In the cluster notebook, cell 30 has a progress bar that shows 415/419 succeeding. Should that read 100%?
  • On a small display, I can see how to see the residual image. The original image takes up the whole screen and there is no slider to pan over to the other frame.
  • When I lock the frames, the original pans at a different rate than the other two (this appears to be because they are at different zoom levels).
  • It would be good to mention that the cluster notebook requires at least a medium size container (the footprint one does too, probably).
  • Since this is a demo, do you want the extra credit portion without providing answers?

I can confirm that my user is able to get to all data necessary to execute the notebook.

I am going to hold off approving until we can iterate on how to organize things to minimize code duplication.

@gpdf
Copy link
Contributor

gpdf commented Apr 1, 2019

@SimonKrughoff Is the data needed for the notebooks of a size and nature compatible with making it available in pop-up cloud deployments? That's one of the expectations of mainstream notebook-demo content, right?

@stargaser
Copy link
Contributor Author

  • I am thinking that these notebooks would show up on prod so users would see them.
    • If that's not desirable, they can just be on master and used for release verification.
    • If they are on prod, we'll want to make sure the data are available in the pop-up deployment case.
  • I will remove the duplicate Firefly.ipynb and I'll have the README link to the top-level one.
  • I'll try to clean up the cluster notebook -- it is the most complicated one because three images are displayed.

@SimonKrughoff
Copy link
Contributor

@gpdf The data used in the notebook is small, but it would need to be extracted from the larger data repository. That is not totally trivial at the moment. This is particularly true for the footprints notebook.

@gpdf
Copy link
Contributor

gpdf commented Jul 1, 2019

This appears to have stalled in the run-up to the LSP review. @yalsayyad was asking about the footprint demo on Slack today, which reminded me about this.

@SimonKrughoff , what has to be fixed before this gets merged to master and then (as I think we want) added to prod? In particular, do we have to get the data subsetting for the footprints notebook done for master? For prod?

(I've pinged @stargaser to take another look at the other cleanups mentioned in the thread above.)

@gpdf
Copy link
Contributor

gpdf commented Jul 1, 2019

Oh, one more consideration. David's notebook points at an output dataset that may be ephemeral. Do we have a general notion of how to handle this situation? Otherwise all tutorials might rot away over time as /datasets evolves. Is there a place for "needed by tutorials" datasets?

@stargaser
Copy link
Contributor Author

  • removed the duplicate Firefly notebook
  • noted that medium container is recommended for deblending notebook; I've run the footprints example in a small container without problems
  • removed exercises from deblending tutorial
  • updated path to HSC footprints data

The deblending notebook is the only one that displays multiple images at the same sky location. I think some improvements to Firefly are on the way for WCS matching and the like.

@SimonKrughoff
Copy link
Contributor

I think the path to the data is still in a location within a large data repository. @frossie may have opinions on merging to master. I don't think subsetting should preclude merging to master, but we really would like the notebooks on prod to be runnable both at the LDF and at GKE for example.

@stargaser
Copy link
Contributor Author

@SimonKrughoff this PR is still stalled on the dataset for the HSC Footprints notebook. If we cannot identify a stable dataset location for this notebook, perhaps the HSC Footprints one should just be deleted.

@SimonKrughoff
Copy link
Contributor

@stargaser can we just merge to master and only port the ones that can run on minimal datasets to prod?

@stargaser
Copy link
Contributor Author

@SimonKrughoff yes, we can just merge to master. I will remove the HSC Footprints notebook. I have worked on adding the footprints overlay to the intro-with-globular.ipynb notebook, but it is not working as intended. IPAC needs to make some decisions about supporting the footprints overlay.

@stargaser
Copy link
Contributor Author

From my hotel WiFi in Thailand, I am unable to start VPN to NCSA to test the notebooks on the LSST Science Platform. I'll hold off on asking for a re-review until I or someone else tests the remaining two notebooks there.

@stargaser
Copy link
Contributor Author

When it is convenient @SimonKrughoff would you please look to see if these can be merged? I was able to run these two notebooks on the lsst-lsp-stable platform after all.

@athornton
Copy link
Member

I've asked @stargaser if this is still relevant.

@gpdf
Copy link
Contributor

gpdf commented Jan 4, 2023

David doesn't work on Rubin/Firefly at this time.

I think we should try to get these notebooks deployed, though.

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.

4 participants