Closed brechmos-stsci closed 5 years ago
I agree to this.
Reading through the text, I have a few comments/suggestions.
@hcferguson I'll change to stars and github issue suggestion
Currently, as I remember, the first two axes have to be spatial and the third spectral. We had not done any re-ordering of the axes when reading in.
For the CUNITS we are setting them to 'deg' if that field is empty or missing from the header. I don't believe we had any IFU data that actually had the CUNIT fields set correctly. I would have to look back at those.
We are comparing the spectral units to a list of valid spectral astropy defined units.
I'll change the yaml file data loader desc.
@hcferguson One other note on the axes and CUNIT parts, we were aiming at being able to load the data we had available to us, which, at that point all were spatial-spatial-spectral and had typically missing units.
But... if you know of other IFU data that might be different (radians or spectral-spatial-spatial), happy to add that into the mix for testing and re-working the loader.
Merging #482 into master will not change coverage. The diff coverage is
n/a
.
@@ Coverage Diff @@
## master #482 +/- ##
=======================================
Coverage 62.34% 62.34%
=======================================
Files 43 43
Lines 5088 5088
=======================================
Hits 3172 3172
Misses 1916 1916
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 159e7b8...5526495. Read the comment docs.
@hcferguson You good with what is here now for the docs? If so, if you could approve the PR. @kassin already said she was good with gist of it.
Looking at the yaml file and reading the docs, my conclusion is that if I have a non-standard file (e.g. with the spectral axis in the first dimension rather than the third, or with wavelength units in furlongs), I'm up a creek. Even a custom loader isn't going to help; I'll have to reformat my data.
If that's the correct conclusion, then I'm fine with the text. If there is some sneaky way with a yaml file to let me specify weird units or a different organization of the array then we ought to document that.
@hcferguson That is the correction assumption (for now). The cubeviz loader will not re-organize. When we have some datasets available that are of an other format then we can re-organize things to be able to load them.
Good to go for this PR.
@kassin and @hcferguson I believe adding some documentation into the read-the-docs is the best way to document this. So, I opened this PR up to start a discussion of how detailed you think this should be from a scientist point of view.
The most basic description is essentially what is in this initial PR but we could make this as long as we wanted as there are lots of caveats etc etc. So.... thoughts on text you think you would like to see here?
Fixes #256