-
Notifications
You must be signed in to change notification settings - Fork 76
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 Gaia catalog to plugin #3090
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3090 +/- ##
==========================================
+ Coverage 88.73% 88.86% +0.12%
==========================================
Files 111 112 +1
Lines 17262 17407 +145
==========================================
+ Hits 15317 15468 +151
+ Misses 1945 1939 -6 ☔ View full report in Codecov by Sentry. |
Address review comments Add max rows option for Gaia
fc156ab
to
4d45bf5
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.
Some improvements in comments below. Other thoughts:
-
Can we add a note somewhere in the UI that this feature uses astroquery's Gaia interface, with a link to the docs? There are lots of configurable features of the Gaia queries that we're not exposing, so referencing the astroquery docs allows us to delegate those docs.
-
Don't forget changelog.
Gaia.ROW_LIMIT = self.max_rows | ||
sources = Gaia.query_object(skycoord_center, radius=zoom_radius, | ||
columns=('source_id', 'ra', 'dec')) | ||
self.app._catalog_source_table = 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.
Can we make this public in the user 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.
Is there a reason making self.app._catalog_source_table
public API belongs in a PR for enabling Gaia catalogs? I'm not really sure why we would do this so genuinely curious.
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.
Looking more closely, I realize that this attr is on app
and not the plugin, and the plugin has a table
attr – I agree with you, let's not do that. Apologies 🙊
from astroquery.gaia import Gaia | ||
from astroquery.gaia import conf | ||
|
||
with conf.set_temp("ROW_LIMIT", self.max_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.
would/should this apply to SDSS as well?
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.
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.
Actually, it does not appear to have an option for that. I just renamed max_sources
to max_gaia_sources
.
@@ -28,6 +28,24 @@ | |||
<g-file-import id="file-uploader"></g-file-import> | |||
</plugin-file-import-select> | |||
|
|||
<v-row v-if="catalog_selected === 'Gaia'"> | |||
<j-docs-link> | |||
See the <j-external-link link='https://astroquery.readthedocs.io/en/latest/gaia/gaia.html' linktext='astropy.gaia docs'></j-external-link> for details on other configurable features. |
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.
but do we allow for passing any of those optional arguments?
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.
We do not, but maybe a particularly knowledgeable user could use the astroquery api to enable them?
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.
ok, since this is UI only, that is easy enough to increment on later if it causes any confusion.
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.
Looks good, thanks @javerbukh! Two suggestions:
Co-authored-by: Brett M. Morris <[email protected]>
Thank you for the reviews and suggestions! I will merge once CI passes. |
Description
This pull request adds the Gaia catalog to the Catalog plugin.
Change log entry
CHANGES.rst
? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rst
before merge. If no, maintainershould add a
no-changelog-entry-needed
label.Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.