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

HDU Header for components #1723

Merged
merged 10 commits into from
Jun 6, 2018
Merged

HDU Header for components #1723

merged 10 commits into from
Jun 6, 2018

Conversation

robelgeda
Copy link
Contributor

This will introduce header files from Data.coords.header into the FITS (1 component/HDU) data exporter. It also adds a function make_component_header that fills in component specific information into the header.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! This is looking good so far, just a few minor comments. In addition, could you add a test?

@@ -28,6 +44,8 @@ def fits_writer(filename, data, components=None):
else:
mask = None

data_header = data.coords.header if hasattr(data.coords, "header") else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more robust way here would be to check if data.coords is an instance of glue.core.coordinates. WCSCoordinates (rather than just checking whether header exists).

@@ -114,14 +114,16 @@ def new_data(suffix=True):
if is_image_hdu(hdu):
shape = hdu.data.shape
coords = coordinates_from_header(hdu.header)
units = hdu.header["BUNIT"] if "BUNIT" in hdu.header else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can write this as hdu.header.get('BUNIT') as that will return None if the key doesn't exist.

@@ -205,7 +207,9 @@ def casalike_cube(filename, **kwargs):
header = hdulist[0].header
result.coords = coordinates_from_header(header)
for i in range(array.shape[0]):
result.add_component(array[[i]], label='STOKES %i' % i)
units = header["BUNIT"] if "BUNIT" in header else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above regarding using .get

expected to come from Data.coords.header by default.
:param component: glue Component
:param header: astropy.io.fits.header.Header
:return:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently trying to switch over to the Numpydoc format (the same one used by astropy), so could you update this docstring accordingly?

@@ -114,7 +114,7 @@ def new_data(suffix=True):
if is_image_hdu(hdu):
shape = hdu.data.shape
coords = coordinates_from_header(hdu.header)
units = hdu.header["BUNIT"] if "BUNIT" in hdu.header else None
units = hdu.header.get('BUNIT')if "BUNIT" in hdu.header else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, you can just do:

units = hdu.header.get('BUNIT')

and you don't need the if statement part.

@robelgeda
Copy link
Contributor Author

Ok thanks. Fixing now

@astrofrog
Copy link
Member

Before I merge this, could you add a test? Thanks!

@robelgeda
Copy link
Contributor Author

@astrofrog I have added some tests

@astrofrog
Copy link
Member

@robelgeda - thanks! is the bunit.fits file actually needed? If not, could you remove it from the last commit?

@robelgeda
Copy link
Contributor Author

It is needed, it is a small 1D fits file with BUNIT inside of it

@astrofrog astrofrog merged commit 4e8cd05 into glue-viz:master Jun 6, 2018
@astrofrog astrofrog added this to the v0.14.0 milestone Oct 18, 2018
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.

2 participants