-
Notifications
You must be signed in to change notification settings - Fork 6
Use query_ssa() for Euclid tutorials 3 & 5 #216
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
base: main
Are you sure you want to change the base?
Conversation
|
@tmesh for some reason I can't tag you to review these changes, but wanted to get your thoughts, and eventually seal of approval since you wrote these tutorials. |
bsipocz
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 have a couple of comments, and one more outstanding todo for myself to see if we can avoid adding the warning handlings.
| @@ -1,15 +1,14 @@ | |||
| --- | |||
| short_title: "SIR 1D Spectra" | |||
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.
Keep this one, please; if jupyerlab removes it automatically then we need to report it upstream; but in the meantime probably need to do patch git add
| from astroquery.ipac.irsa import Irsa | ||
| #suppress warnings about deprecated units |
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.
TODO for bsipocz: check if we need this or something has been shipped upstream already; but even if it did it would very likely be in a very recent release so we can't cleanup just yet.
| @@ -1,5 +1,4 @@ | |||
| --- | |||
| short_title: "SPE Catalogs" | |||
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.
keep the short title
bsipocz
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.
Thank you!
I bunch of smaller comments, the most critical ones is git add changes in batches so we leave the metadata alone as we need that for the rendering.
Co-authored-by: Brigitta Sipőcz <[email protected]>
|
@bsipocz I will try my best to do the adding in patches for future PRs, I know you have mentioned this before, its just not yet part of my muscle memory. For this PR I have manually added back in the shortitles since that seemed easier than messing with the commit history. I believe all the other changes are done. |
bsipocz
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.
All looks good to me, but I don't merge it myself in case you want to wait for Tiffany's feedback.
Mostly just updated to use the IRSA preferred query_ssa() to look for spectra.
For notebook 3, I switched from searching for a spectrum by objectID to searching by coords. I feel this is a more generically useful approach.
Minor other changes:
This will close #141