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

Add URAT catalog information #526

Merged
merged 31 commits into from Aug 6, 2019
Merged

Add URAT catalog information #526

merged 31 commits into from Aug 6, 2019

Conversation

geordie666
Copy link
Contributor

This PR adds the URAT catalog to desitarget. It includes:

  • A new module uratmatch.py to retrieve URAT data from Vizier/FTP and reformat it.
  • Code to efficiently match RAs/Decs to URAT, as part of the uratmatch.py module.
  • Code to substitute URAT proper motions for GFAs where Gaia has not yet measured proper motions (the other Gaia information is retained for a GFA target, but zero/NaN proper motions are overwritten with URAT values, and the URAT_ID and separation is recorded).

This addresses #520. Here's an example of a general region we uncovered that had zero objects in a guide camera, which now has multiple objects from URAT:
URATGFAs

Copy link
Contributor

@sbailey sbailey left a comment

Choose a reason for hiding this comment

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

Structural quibbling: It seems odd to have a py/desitarget/fortran/ directory (i.e. fortran under python).
I certainly hope we aren't planning on adding more fortran. I would like this better
if everything URAT related was in a py/desitarget/urat directory, or if the fortran code
was moved out of py/desitarget and into a top level fortran/ or urat/ or
etc/urat/ directory.

Is having a copy of the URAT catalog processed by v1dump now required to be able to run gfa_targets? Is that what we want? For production yes; might be a hassle for development, though I'm not aware of anyone running this part of desitarget away from NERSC anyway. Just raising it for consideration.

A few other small comments inline.

py/desitarget/uratmatch.py Show resolved Hide resolved
py/desitarget/fortran/README Outdated Show resolved Hide resolved
@geordie666
Copy link
Contributor Author

Note that it will now be required to have the URAT catalog to run GFAs. But, it was already required to have the suitably-formatted Gaia catalog to run GFAs. The second (Gaia) is a far greater burden that the first (URAT), so I'm comfortable with it being a requirement.

@geordie666
Copy link
Contributor Author

I'd be 100% against moving the Fortran directory to the top level. I don't want to give the impression that we're expecting to add more Fortran code. In other words, Fortran is a "special case" under Python, here. The Python is a wrapper on it. Moving the Fortran to a top-level directory would imply that it has equal and substantial weight.

I also don't want to move everything into a urat directory, which would beg the question of why the gaiamatch code is in the py directory but the uratmatch code is in py/urat/. That seems structurally weird, to me.

@geordie666
Copy link
Contributor Author

geordie666 commented Aug 3, 2019

@sbailey: I believe I addressed all of your requests. I also:

  • moved the fortran code to etc/fortran as I liked that suggestion best (I've just noticed you actually suggested etc/urat. Let me know if you feel strongly about that distinction).
  • added a --nourat command line argument in the binary, so if people really don't want to run without URAT, they don't have to.

I'm running a few tests of these changes. I'll let you know when I'm done.

@geordie666
Copy link
Contributor Author

@sbailey: OK, I'm done with internal tests. Let me know if you want more changes.

@sbailey
Copy link
Contributor

sbailey commented Aug 6, 2019

Looks good; thanks. Merging now.

@sbailey sbailey merged commit 88ff5e8 into master Aug 6, 2019
@sbailey sbailey deleted the ADM-URAT-GFAs branch August 6, 2019 23:56
qmxp55 pushed a commit to qmxp55/desitarget that referenced this pull request Feb 12, 2020
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.

None yet

2 participants