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

U/jrbogart/gaia bypass #90

Merged
merged 16 commits into from
Apr 29, 2024
Merged

U/jrbogart/gaia bypass #90

merged 16 commits into from
Apr 29, 2024

Conversation

JoanneBogart
Copy link
Collaborator

Optionally read Gaia data from FITS files directly rather than via Butler.
Extract region masking utility to shapes.py; eliminate old version in skyCatalogs.py
Eliminate main program from skyCatalogs.py. It is largely superseded by tests/test_API.py

Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

I didn't try running this since the mismatch between coord_[ra,dec] units and the assumed units in the masking code looks fatal. I'd recommend running this on the data and checking that the outputs make sense and adding test code if possible.

f = open(f_path)
f.close()
except FileNotFoundError:
if not silent:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems better suited for using a logger and having the log level determine whether a message is emitted if the file is not found.

Comment on lines 157 to 163
try:
f = open(f_path)
f.close()
except FileNotFoundError:
if not silent:
print(f'No file for requested htm id {htm_id}')
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it would be better to do something like

if not os.path.isfile(f_path):
    logger.info("No file for requested htm id %s", htm_id)
    return

instead of this try/except?

Comment on lines +172 to +173
ra_full = tbl.get('coord_ra')
dec_full = tbl.get('coord_dec')
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm pretty sure coord_ra and coord_dec are in radians in the refcat files-- see the conversion in GaiaObject.__init__. If so, these need to be converted to degrees when computing the masks below.

@JoanneBogart
Copy link
Collaborator Author

Yes, I need to change to degrees for the masking; that probably explains some strange things I saw while testing. I have some code which fetches the data using old and new methods. It will take some work to get it to do a comparison.

Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

I left a small comment regarding a minor suggestion for the test code, otherwise lgtm.

{'collections': 'refcats',
'dstype': 'gaia_dr2_20200414',
'repo': os.path.join(CATALOG_DIR, 'repo')},
'data_file_type': 'butler_refcat'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit confusing to see data_file_type set as 'butler_refcat' here when this test code is mean for direct FITS access. Unlike test_gaia_objects.py, data_file_type doesn't need to be set since this version only reads in the data directly into self.df, so I think it would be clearer to omit this and maybe note that this CONFIG dict is just used by get_gaia_data.

@JoanneBogart JoanneBogart merged commit dc18087 into main Apr 29, 2024
1 check passed
@JoanneBogart JoanneBogart deleted the u/jrbogart/gaia_bypass branch April 29, 2024 18:12
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