(minimal) user API for catalog search #1636
Conversation
Codecov ReportBase: 86.32% // Head: 86.32% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
- Coverage 86.32% 86.32% -0.01%
==========================================
Files 95 95
Lines 9596 9607 +11
==========================================
+ Hits 8284 8293 +9
- Misses 1312 1314 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kecnry
force-pushed
the
api-catalog-search
branch
from
3c66a92 to
310ccb5
Compare
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
pllim
as code owners
|
The plugin is still in "experimental" mode and missing many features. Should we shelf this until it is more stable? |
|
If we think there is high likelihood of changing any of the API exposed here, then I think that's a good idea to wait (although I think some API-compatibility improvements are still worth merging here, I'd just remove the |
|
👍 to hiding user API for now. When we revisit this plugin, there is a small chance we will refactor the design (e.g., if people want to upload their own catalogs and so on). |
kecnry
force-pushed
the
api-catalog-search
branch
from
b6c31c6 to
eb90326
Compare
|
ok, I've removed the exposed functionality, but think the rest is still worth getting in (maybe the PR title needs a change though to avoid us thinking it was already done here? - I also created a follow-up ticket 🐱 so we don't forget). |
I dunno, let's ask @ojustino 😸 |
kecnry
changed the title
| @@ -111,6 +111,9 @@ Plugins | |||
| .. automodapi:: jdaviz.configs.imviz.plugins.aper_phot_simple.aper_phot_simple | |||
| :no-inheritance-diagram: | |||
|
|
|||
| .. automodapi:: jdaviz.configs.imviz.plugins.catalogs.catalogs | |||
There was a problem hiding this comment.
That's weird. I remember suggesting this to Sabrina's PR but maybe it was lost in rebase. Thanks for adding this.
| @@ -111,12 +125,13 @@ def search(self): | |||
| viewer.marker = {'color': 'red', 'alpha': 0.8, 'markersize': 5, 'fill': False} | |||
| viewer.add_markers(table=catalog_results, use_skycoord=True, marker_name='catalog_results') | |||
|
|
|||
| return skycoord_table | |||
There was a problem hiding this comment.
yes, we don't set it as a traitlet anywhere or show it in the plugin UI, so I figured you might as well be able to access the table of results from the API method instead of having to dig into markers. This technically won't be public yet now that I stripped out exposing it, so if you'd prefer, I can revert this change for now too.
There was a problem hiding this comment.
No, I think this is good to have. Just want to make sure. Thanks!
kecnry
force-pushed
the
api-catalog-search
branch
from
eb90326 to
4f21270
Compare
Description
This pull request extends on #1401 and provides user-API access to the catalog search plugin.
Plugin User API docs
TODO:
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.
triviallabel.CHANGES.rst?