-
Notifications
You must be signed in to change notification settings - Fork 8
FIREFLY-1862: Cleanup the firefly-client docs and example notebooks #76
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
Conversation
Remove duplicate files
More restructure of test notebooks and cleanup
Fix minor bug/docs in firefly_client
Cleanup and execute other notebook doc pages
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Fix nbsphinx syntax highlighting for code cell
robyww
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jaladh-singhal gave me an overview. It looks good.
stargaser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only looked at a few parts, but it looks good to me.
|
Overall I think these docs are really good. There are well structured, clear, and I got nearly all of the examples to run. I especially liked the 3-color image display and the ds9 regions capability, which I didn't know about previously. There are just a few places where I think the docs could be improved (see below). Initializing:
Displaying images:
Displaying 3-color images:
Visualizing tables:
|
|
@jonesmg thank you for your feedback. It's very useful! Please see my replies to your comments.
Hmm good point. I think this prompts a discussion with developers (and scientists?) to come up with right set of pros/cons.
I'll add.
We don't have installation instructions dedicatedly. Maybe I should have followed format of irsa-tutorials notebooks and added a
Oh, good catch - I wasn't aware. Is this because you had numpy installed in your environment before you installed astropy? I think I created a fresh environment with just firefly-client and astropy so numpy (as a dependency of astropy) was in the right version.
Oh it's because
I'm not sure if I completely understand how you make the third image on-the-fly. Can you share a code snippet? I'm assuming that you do some aggreegation function on the data of two fits images as numpy array, in that case you can directly supply it to firefly as IO stream instead of writing it to disk.
Yes, this was an issue in Irsaviewer that I fixed very recently (today's patch release). Please try again it should be working. |
|
Thanks for the responses Jaladh. Some follow-up points below
That seems reasonable to me, except possibly for the numpy=2.3 issue below, depending on how we address that.
I'm not sure exactly why I'm having this issue and you are (I'm running on an M2 Macbook). I deleted my environment and started from scratch, doing the following: conda create -n firefly python=3 astropy jupyterlab
conda activate firefly
pip install firefly_client
export FIREFLY_URL="https://irsa.ipac.caltech.edu/irsaviewer"
jupyter labWhen I tried to import the modules from astropy.convolution I hit the error: AttributeError: module 'numpy' has no attribute 'in1d' conda create -n firefly python=3 numpy=2.3 astropy jupyterlabThen all the astropy stuff imports.
Thanks for clarifying that, I hadn't realised that detail, but I think it's my fault for not reading carefully enough. It does work now (although, it was quite slow, my images were each >100 MB -- maybe this is one of the reasons to use a local server).
Sure, here a code snippet of when I've done this for a random HST image: F606Wfits = fits.open(data_dir+F606Wfitsname)
F606W = F606Wfits[1].data
F814Wfits = fits.open(data_dir+F814Wfitsname)
F814W = F814Wfits[1].dataI omitted some code here that just aligns the two images etc.
This all works now! |
Fixes FIREFLY-1862
examples/contents: added meaningful names/titles, merged/deleted duplicate notebooks, moved deprecated and testing notebooks to a sub directory, etc.test/directory has now becomeexamples/development_testssince that's what they are, they are not unit tests but integration tests in form of notebooks or scriptsexamples/(all notebooks and scripts, except those in subdirectories) render as html pages in the docs website for easy view and download by userconf.pyand adding a custom extensionscript_pages.py- we can get rid it when improving infrastructure to use myst-nb instead of nbsphinx (notebook->HTML renderer libraries) similar to https://github.com/Caltech-IPAC/irsa-tutorialsdocs/usage/*.rstwhich were outdated to.ipynbfiles with up-to-date API usagefirefly_client.pyTesting
See docs built from this PR-branch at https://caltech-ipac.github.io/firefly_client/pr/76/
Compare them against current docs (built from master branch): https://caltech-ipac.github.io/firefly_client/