-
-
Notifications
You must be signed in to change notification settings - Fork 418
GAIA: new simplified cross match method #3320
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
GAIA: new simplified cross match method #3320
Conversation
a80c138
to
dc0fac9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3320 +/- ##
==========================================
+ Coverage 69.88% 69.97% +0.08%
==========================================
Files 232 232
Lines 19761 19814 +53
==========================================
+ Hits 13810 13864 +54
+ Misses 5951 5950 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
49dbbfe
to
34b324d
Compare
7fb5b9d
to
e63ae6b
Compare
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.
Thanks! Overall looks good, I just have minor comments.
And please rebase so it picks up the recent fixes and will pass the RTD build.
@@ -853,15 +848,155 @@ def load_user(self, user_id, *, verbose=False): | |||
|
|||
return self.is_valid_user(user_id=user_id, verbose=verbose) | |||
|
|||
def cross_match_basic(self, *, table_a_full_qualified_name, table_a_column_ra, table_a_column_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 didn't notice this weirdness in the argument names before, e.g. why do we need qualified
in there, what does it even mean?
Anyway, I see that it's some historical heritage that we haven't spotted before, so let's keep it to have a consistent 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.
"qualified" means that the name of the table must contain the schema. Something like "my_schema.my_table"
astroquery/gaia/core.py
Outdated
#. updates the user table metadata to flag the positional RA/Dec columns; | ||
#. launches a positional cross-match as an asynchronous query; | ||
#. returns all the columns from both tables plus the angular distance (deg) for the cross-matched sources. |
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.
Unfortunately this won't be auto resolved into numbers in the docstrings, only when sphinx rendered, so could you please instead spell out the numberings?
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.
Done. Changes committed to the branch.
astroquery/gaia/core.py
Outdated
the ‘dec’ column in the table table_b_full_qualified_name | ||
results_name : str, optional, default None | ||
custom name defined by the user for the job that is going to be created | ||
radius : float (arc. seconds), optional, default 1.0 |
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 think it would be useful to accept Quantity radius, too, not just floats
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.
The methods cross_match and cross_match_basic now accept radius as a Quantity
09f2f1b
to
f7948be
Compare
Branch rebased. |
astroquery/gaia/core.py
Outdated
step: | ||
1. updates the user table metadata to flag the positional RA/Dec columns; |
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 suspect this lack of an empty line is the cause for the sphinx failure
step: | |
1. updates the user table metadata to flag the positional RA/Dec columns; | |
step: | |
1. updates the user table metadata to flag the positional RA/Dec columns; |
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.
Done. Changes committed to the branch.
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.
Thanks! I'll investigate what goes wrong with the oldest and dev builds, I'm certain it is unrelated, yet it's something new and concerning so I rather check them out first to be sure.
c05573e
to
2676412
Compare
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.
Thanks!
CI failures are unrelated and have been fixed in main
, thus I go ahead with the merge as is.
Dear astroquery team,
The current implementation of the astroquery.gaia built-in cross-match tool relies in a sub-optimal intermediate ADQL query. Furthermore (and after uploading a table to the user space), it requires a 3-step approach involving different functionalities (each of them requiring multiple "user clicks"):
We propose to streamline this procedure to improve the user experience and hopefully promote the usage of this functionality by updating the current implementation.
The proposed mechanism:
We have developed the new method cross_match_basic as well as new tests. This change is backward compatible with the present implementation of the cross-match method.
cc @esdc-esac-esa-int
jira: GAIASWRQ-25