-
Notifications
You must be signed in to change notification settings - Fork 46
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
Add hermes spectro data sharing #956
Conversation
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.
Made a few comments. Happy to talk about them in more detail if necessary.
Another thing to consider: There aren't currently many ways to access this spectroscopy sharing for Hermes. We should probably add spectra data to the main sharing page target_share.html
and the spectroscopy tab of the target_detail
page.
'instrument': datum.value.get('instrument', settings.DATA_SHARING['hermes'].get('DEFAULT_INSTRUMENT', '')), | ||
'flux': flux_list, | ||
'wavelength': wavelength_list, | ||
'flux_units': datum.value.get('flux_units', settings.DATA_SHARING['hermes'].get('DEFAULT_FLUX_UNITS', 'mJy')), |
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.
We should do a check here for classic astropy units that have been converted to strings and convert them into what Hermes is looking for. (it probably makes more sense to accept astropy units in hermes and do any conversions there, just like we do for dates, but that might be outside the scope of this PR.)
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.
So I added conversion functions for the units that existed in astropy, but none of them overlapped for flux... Unless I'm missing a way in the astropy units to denote like milli-, giga-, terra- to a unit. Also the default flux unit you use in the dataprocessor in the TOM is one we support in hermes but doesn't appear to be a unit in astropy unless I am missing it - it is an amalgamation of 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.
yeah, so a combined unit is still recognized as a unit. For example:
q = 42 * u.erg / u.s / u.cm / u.cm / u.Angstrom
q.unit
resolves to
Unit("erg / (Angstrom s cm2)")
and mJy
is a valid and understood astropy unit.
I've addressed most of your comments. It appears to me we just show a single plot of all the spectras on it in the spectra page. Should I add a button to share them all then, since we don't have any breakdown of them individually like in snex2? Honestly tom_base should probably pull in the snex2 spectra page to show the combined and all the individual ones with metadata since that seems more useful. |
I think implementing a SNex2 like spectra page is definitely the way to go. (See #861 ) |
…_hermes_spectro_sharing
I've added a fix for the data sharing download functionality to work with spectrograph type reduced datums |
This adds support for spectra ReducedDatum hermes sharing in two formats, one of which is the format snex uses to store their spectra data.
Also renamed some html elements on the photometry datalist since i have to have similar code in snex2 for a spectroscopy datalist and they are all loaded into a single page, so better to be more specific with the javascript function names and stuff.