Skip to content
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

updated BUNIT to proper units #284

Merged
merged 7 commits into from Oct 11, 2016
Merged

updated BUNIT to proper units #284

merged 7 commits into from Oct 11, 2016

Conversation

rstaten
Copy link
Contributor

@rstaten rstaten commented Oct 10, 2016

This PR:

@sbailey
Copy link
Contributor

sbailey commented Oct 10, 2016

Thanks! This PR fixes the BUNIT for fluxcalib output. Could you also add BUNIT for cframe? In that case I think it is completely missing.

@rstaten
Copy link
Contributor Author

rstaten commented Oct 10, 2016

It is completely missing, but the cframe files seem to be generated using the same write_frame function as the frame files. Since these two files do not have the same output flux units, this may be why the BUNIT keyword is not included. If desired, I could simply write a new function or change the current one to distinguish between the outputs.

Also, I'm assuming the stdstars output should also include the factor of 1e-17 as well? If so, this is a simple fix.

@sbailey
Copy link
Contributor

sbailey commented Oct 10, 2016

It is completely missing, but the cframe files seem to be generated using the same write_frame function as the frame files. Since these two files do not have the same output flux units, this may be why the BUNIT keyword is not included. If desired, I could simply write a new function or change the current one to distinguish between the outputs.

Good point. Let's add an optional units keyword to desispec.io.write_frame, with something like

if units is not None and 'BUNIT' not in hdr:
    hdr['BUNIT'] = str(units)

That should work for units as a string and if units is an astropy.units.Units object. To check: are there any python 3 unicode gotchas? I hope not...

And then update the code that calls desispec.io.write_frame with the proper units for frame vs. cframe.

Also, I'm assuming the stdstars output should also include the factor of 1e-17 as well? If so, this is a simple fix.

Yep.

@rstaten
Copy link
Contributor Author

rstaten commented Oct 10, 2016

I have set the BUNIT for frame files to photons/bin and cframe files to 1e-17 erg/s/cm^2/A.

@sbailey
Copy link
Contributor

sbailey commented Oct 11, 2016

We had a misunderstanding about my suggestion for how to propagate units to the frame files. Implementing it was easier than re-describing it, so I went ahead and changed it. Since we do propagate header keywords from inputs to outputs, I would like to run an integration test on this to check if we end up with any intermediate steps with incorrect BUNITs. i.e. don't merge yet; I'll fix or report any problems, then merge.

@sbailey
Copy link
Contributor

sbailey commented Oct 11, 2016

I made a number of small changes:

  • Added more BUNIT throughout
  • Made BUNIT strings that can be parsed by astropy.unit.Unit (even if we aren't doing that in the main code). e.g. "Angstrom" instead of "A" and "electron" instead of "electrons".
  • fixed fiberflat single- vs. double-precision when reading in (unrelated to original purpose of this PR)
  • fixed fiberflat EXTNAME (now "FIBERFLAT" not "FLUX"; unrelated to original purpose of this PR)

I checked this with an integration test to make sure that BUNITs weren't incorrectly propagated from one file type to another. After the Travis tests pass I think this is ready to merge. Thanks @rstaten for getting this started.

@sbailey sbailey merged commit 4ae5b69 into master Oct 11, 2016
@sbailey sbailey deleted the fluxcalib_bunit branch October 11, 2016 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cframe files should set BUNIT keyword
2 participants