Skip to content

Add SSA and SCS Support to VO Loader#4060

Open
duytnguyendtn wants to merge 13 commits into
spacetelescope:mainfrom
duytnguyendtn:vossa
Open

Add SSA and SCS Support to VO Loader#4060
duytnguyendtn wants to merge 13 commits into
spacetelescope:mainfrom
duytnguyendtn:vossa

Conversation

@duytnguyendtn
Copy link
Copy Markdown
Collaborator

@duytnguyendtn duytnguyendtn commented Mar 3, 2026

Description

This PR extends the VO Loader to add support for IVOA's Simple Spectrum Access protocol and allow Jdaviz to load spectra, and Simple Catalog Search for Catalogs respectively. This is done by adding a dropdown at the very top which requests if users are interested in searching for Images or Spectra, which selects the appropriate protocol to pass to pyvo under the hood.

image image

Tests in progress

Change log entry

  • Is a change log needed? If yes, is it added to 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, maintainer
    should 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.

  • Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
  • Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
  • Do the proposed changes follow the STScI Style Guides?
  • Are tests added/updated as required? If so, do they follow the STScI Style Guides?
  • Are docs added/updated as required? If so, do they follow the STScI Style Guides?
  • If new remote data is added that uses MAST, is the URI added to the cache-download.yml workflow?
  • Did the CI pass? If not, are the failures related?
  • Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
  • After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

@duytnguyendtn
Copy link
Copy Markdown
Collaborator Author

I pushed up my code as-is to get feedback, as I couldn't get this particular Vuetify trick to work. Any Vuetify expert advise would be greatly appreciated!

My original plan was to have the initial dropdown ask the user for "Images" or "Spectra" (that text exactly), but have the underlying value be "sia" or "ssa" respectively for service selection down the line. Based on the Vuetify 2 docs, I should be able to accomplish it this way:

servicetype_choices = List(
[
{"label": "Images", "value": "sia"},
{"label": "Spectra", "value": "ssa"},
]
).tag(sync=True)

with a small change of swapping "text" with "label" to accommodate Jdaviz's custom plugin-select component defined here:

item-text="label"
item-value="label"

But therein lies a problem: The plugin-select component manually forces the item-value and item-text to the same value. This raises the first question: Is there a reason why we force the two to be identical? If I change this, it will probably break things... right?

I also cannot tell if this is something that is overridable by the specific component I'm writing. Setting the item-value='value' in the VO Loader doesn't seem to do anything for me. If I do try and change it all the way in here, it also don't produce sensible output. So I can't tell if we can't override, or if my overriding is failing because of something else hidden/tucked away somewhere...

@kecnry
Copy link
Copy Markdown
Member

kecnry commented Mar 5, 2026

we could do that, but then its confusing which (the label or the value) is expected for the user from the API for the select component. Can we just keep the user-friendly label in the UI and API and then map to what is needed for the backend call with an if-statement or dictionary lookup (or perhaps by setting 'object' in the dictionary or similar so the .selected_obj infrastructure will handle the lookup in one place)?

@duytnguyendtn
Copy link
Copy Markdown
Collaborator Author

its confusing which (the label or the value) is expected for the user from the API

Oh okay I understand the catch 22 we're under here...

map to what is needed for the backend call with an if-statement or dictionary lookup

We definitely can; it just seems like a shame to have to encode this logic if there's a built-in mechanism in Vuetify for this specific purpose. But in lieu of any better options, we might be boxed into this solution... Oh well, I'll rework that logic in this manner hopefully to sidestep this problem

@duytnguyendtn duytnguyendtn changed the title Add Simple Spectrum Access Support to VO Loader Add SSA and SCS Support to VO Loader Mar 18, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 18, 2026

Codecov Report

❌ Patch coverage is 54.54545% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.60%. Comparing base (d237b13) to head (d8a373b).

Files with missing lines Patch % Lines
...z/core/loaders/resolvers/virtual_observatory/vo.py 50.00% 5 Missing ⚠️

❌ Your patch check has failed because the patch coverage (54.54%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4060      +/-   ##
==========================================
+ Coverage   84.57%   84.60%   +0.02%     
==========================================
  Files         205      205              
  Lines       30414    30418       +4     
==========================================
+ Hits        25724    25734      +10     
+ Misses       4690     4684       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@duytnguyendtn duytnguyendtn added this to the 5.1 milestone Apr 15, 2026
@duytnguyendtn duytnguyendtn marked this pull request as ready for review April 15, 2026 21:40
@duytnguyendtn
Copy link
Copy Markdown
Collaborator Author

duytnguyendtn commented Apr 15, 2026

I don't think the failing tests are my doing; they seem to appear independent. At least the tests I added are passing locally for me. I'll provide this PR for review now!

Comment thread jdaviz/core/loaders/resolvers/virtual_observatory/vo.vue
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.

2 participants