Extend spectrum information written by tabular_fits_writer
#539
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nmearl
reviewed
Nov 7, 2019
| header.update([k for k in spectrum.meta.items() if | ||
| isinstance(k[1], hdr_types)]) | ||
|
|
||
| # Strip header of FITS reserved keywords (strangely, loop does not work!) |
There was a problem hiding this comment.
Alternatively, you could do:
for keyword in ['NAXIS', 'NAXIS1', 'NAXIS2']:
header.remove(keyword, ignore_missing=True)|
|
||
| columns = [disp, flux] | ||
| colnames = [dispname, "flux"] | ||
| # Include uncertainty - ToDo: uncertainty units? |
There was a problem hiding this comment.
I think it's alright to not deal with uncertainty units because they should be assumed the same as the flux units.
| colnames = [dispname, "flux"] | ||
| # Include uncertainty - ToDo: uncertainty units? | ||
| if spectrum.uncertainty is not None: | ||
| columns.append(spectrum.uncertainty.array * spectrum.uncertainty.unit) |
There was a problem hiding this comment.
Suggested change
| columns.append(spectrum.uncertainty.array * spectrum.uncertainty.unit) | |
| columns.append(spectrum.uncertainty.quantity) |
|
Thanks for the suggestions, I think they make for notably more concise code! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is an attempt to address some of the issues reported in #531, notably adding the missing unit information to the written spectrum. It also adds the
uncertaintycolumn, if present, and indicates the type ofspectral_axisinTTYPE1. The last commit adds writing of any other header metadata, taking the 2nd option discussed in#531 (comment) (with a new kwarg
update_header). It also passes other kwargs though to the table writer, allowing e.g. to force overwriting of existing files.Added tests for all these functions in
test_loaders.pyFiles thus written still fail to be read in as
tabular-fitson theWCSWrappererror, but since they are fully compliant (except for a missing checksum) FITS files with complete spectral information, this really seems to be more of a problem on the reader side.