-
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
| ```{code-cell} ipython3 | ||
| # Uncomment the next line to install dependencies if needed | ||
| # !pip install matplotlib astropy 'astroquery>=0.4.10' | ||
| !pip install matplotlib astropy 'astroquery>=0.4.10' |
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 comment
| !pip install matplotlib astropy 'astroquery>=0.4.10' | |
| # !pip install matplotlib astropy 'astroquery>=0.4.10' |
| 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
| coord_obj = SkyCoord( | ||
| ra=float(obj_row["ra"][0]) * u.deg, | ||
| dec=float(obj_row["dec"][0]) * u.deg, | ||
| ) |
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.
Simplify it:
| coord_obj = SkyCoord( | |
| ra=float(obj_row["ra"][0]) * u.deg, | |
| dec=float(obj_row["dec"][0]) * u.deg, | |
| ) | |
| coord_obj = SkyCoord(obj_row["ra"][0], obj_row["dec"][0], unit=u.deg) |
Also, why do we cut this for just the first one?
| ``` | ||
|
|
||
| ## About this Notebook | ||
| ###### About this Notebook |
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 think you need to keep the heading level her
| ###### About this Notebook | |
| ## About this Notebook |
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.
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