-
Notifications
You must be signed in to change notification settings - Fork 5
Update Euclid spectra calls for new SpectrumDM service #137
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
jaladh-singhal
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 the changes look good to me!
e37ad9e to
f796d94
Compare
|
IRSA has released the SpectrumDM updates to ops. I've tested this branch and all looks good. |
| - WAVELENGTH is in Angstroms by default | ||
| - SIGNAL is the flux and should be multiplied by the FSCALE factor in the header | ||
| - WAVELENGTH is in Angstroms by default. | ||
| - SIGNAL is the flux. The scale factor is included in the 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.
There is now no "scale factor" anywhere in this notebook, so I would suggest rephrasing and/or extending the explanation a bit more.
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.
Expanded the description.
| ```{code-cell} ipython3 | ||
| file_uri = urllib.parse.urljoin(Irsa.tap_url, result['uri'][0]) | ||
| file_uri | ||
| spectrum_path = f"https://irsa.ipac.caltech.edu/{result['path'][0]}" |
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 do still feel that we should get the base url from somewhere, programatically, and not hard-wired.
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 agree that would be better but I have no idea where to get that from. What do you suggest?
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.
open an issue in astroquery, and eventually we'll have it hardwired there.
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.
| ``` | ||
|
|
||
| In above table, we can see that the `uri` column gives us location of spectra file on IBE. We can map it to S3 bucket key to retrieve spectra file from the cloud. This is a very big FITS spectra file with multiple extensions where each extension contains spectrum of one object. The `hdu` column gives us the extension number for our object. So let's extract both of these. | ||
| In above table, we can see that the `path` column gives us a url that can be used to call the SpectrumDM service to get the spectrum of our object. We can map it to an S3 bucket key to retrieve a spectra file from the cloud. This is a very big FITS spectra file with multiple extensions where each extension contains spectrum of one object. The `hdu` column gives us the extension number for our object. So let's extract both of these. |
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.
a url that can be used to call the SpectrumDM service
Is SpectrumDM a service though? My understanding is that it's the data model and that the service will take a fits file url and turn it into a votable that follows the SpectrumDM standard -- nitpicking, but may be worth being clear and explicit in the explanation
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.
Switched it to be generic and just say, "call an IRSA service".
|
Look at the rendered notebook, there is a KeyError in there. |
|
KeyError was from a different notebook that I didn't realize also needed to be updated for the SpectrumDM release. So, yay CI for catching it. I pushed a fix. |
|
(closed--reopened to retrigger CI. Is GHA htmlbuild pass, we can override the branch protection even without circleCI) |
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.
One more nitpick, but overall looks good to me.
Thanks
IRSA-7280
Starting on Wednesday, Sept 24, TAP calls for table 'euclid.objectid_spectrafile_association_q1' will return a
'path'column instead of a'uri'column and it will contain a SpectrumDM url that will return a single spectrum instead of the full MEF. This PR updates our notebooks to accommodate.To test: Before running any notebooks, create or edit the file
~/.astropy/config/astroquery.cfgand make sure it has at least the following lines in it. Also make sure you're connected to IPAC VPN.