-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
…nternal routine Eliminate old unused main test program. Contents are largely in test_API
There was a problem hiding this 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.
skycatalogs/objects/gaia_object.py
Outdated
f = open(f_path) | ||
f.close() | ||
except FileNotFoundError: | ||
if not silent: |
There was a problem hiding this comment.
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.
skycatalogs/objects/gaia_object.py
Outdated
try: | ||
f = open(f_path) | ||
f.close() | ||
except FileNotFoundError: | ||
if not silent: | ||
print(f'No file for requested htm id {htm_id}') | ||
return |
There was a problem hiding this comment.
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?
ra_full = tbl.get('coord_ra') | ||
dec_full = tbl.get('coord_dec') |
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this 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.
tests/test_gaia_direct.py
Outdated
{'collections': 'refcats', | ||
'dstype': 'gaia_dr2_20200414', | ||
'repo': os.path.join(CATALOG_DIR, 'repo')}, | ||
'data_file_type': 'butler_refcat'} |
There was a problem hiding this comment.
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
.
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