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

Virtual Observatory plugin for Jdaviz #2872

Open
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

duytnguyendtn
Copy link
Collaborator

@duytnguyendtn duytnguyendtn commented May 10, 2024

Description

NOTE: This PR depends on https://github.com/astropy/pyvo/tree/main until it is released with the changes introduced in astropy/pyvo#594

This PR introduces a new plugin to query resources and load data products published by collaborators of the International Virtual Observatory Alliance. This Plugin currently queries resources implementing the original 1.0 version of the Simple Image Access protocol by specifying a source and a waveband and loads the selected products into Imviz. Though currently only implemented into Imviz, I placed this plugin into the default config in hopes this plugin can be expanded to also work for Specviz via the Simple Spectrum Access Protocol, and Cubeviz as well. Documentation and Unit Tests have been implemented, and this is formally ready to review!

image

Additionally, I placed the VO plugin closer to the top mainly for my own testing, but also because loading data generally occurs before data analysis. Feel free to adjust the location of the plugin within the plugin bar to your liking.

Thanks for your inputs/help getting us this far! We at the HEASARC hope this is a useful contribution to the Jdaviz tool and community :)

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?
  • 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)? 🐱

@github-actions github-actions bot added imviz plugin Label for plugins common to multiple configurations labels May 10, 2024
@duytnguyendtn
Copy link
Collaborator Author

What is the objection of pixel linking anyway?

I don't personally have one, but I'm following through with a request from @camipacifici to block users from loading VO data products unless WCS linking is enabled. Maybe Cami can provide insight to the requirement here?

@camipacifici
Copy link
Contributor

If there is no data at all, I agree that it does not matter. I am thinking more the case where the user has already loaded an image then looks in the VO plugin to find the same field from another telescope. If the image is loaded when linking is by pixel, the user could be confused because the images will not match. Personally, I would not automatically toggle WCS linking, but I would add an orange box that prompts the user to go toggle the linking to WCS.

@camipacifici
Copy link
Contributor

Sorry, I just reread your previous message. I do not want to block the user from loading data. I just want to warn them that they will not see the images aligned if they do not link by WCS.

@duytnguyendtn
Copy link
Collaborator Author

@camipacifici I just pushed up 10d712d which provides a warning when:

  1. WCS linking is not enabled and
  2. There is pre-existing data in the data collection

On point 2, I figured a warning about misaligned data would be nonsensical if there's only one dataset total (the one the user is loading). Give it a try and see if it's the behavior you're expecting. Thanks!

@camipacifici
Copy link
Contributor

I pulled your latest version here but I do not see the warning. Maybe because I had the plugin already open when I loaded data?

Here is a case that returns a problem:
Screenshot 2024-08-01 at 4 34 59 PM
Screenshot 2024-08-01 at 4 35 22 PM
Any clues? It is also still thinking for some reason, but the rest of the tool is responsive.

@duytnguyendtn
Copy link
Collaborator Author

Hi Cami,

On the smaller points, I pushed up a change that should fix the "hanging" issue. I forcibly stopped the plugin whenever a load data error occurred, but forgot to properly clear the loading flag. Instead, now a load_data error should not stop the plugin; it will notify via snackbar message and then move on to other selected files.

On the lack of linking error message, that one concerns me; let me explicitly spell out what conditions it should hit and you let me know which path you were in, or if you hit a case I'm not covering:

  1. If there is NO data loaded, then the VO plugin loads the first data entry into the data collection, no error message should appear, regardless of link type
  2. If there is already data in the data collection then the VO plugin loads another data, and pixel linking is on, the warning snackbar should appear
  3. If there is already data in the data collection then the VO plugin loads another data, and pixel linking is off, no warning should appear.

I have a test that checks for these conditions in CI, which is passing, so I'm concerned you may have found a scenario I'm not covering for. If you are in one of the above, could you detail what was in your data_collection when you tried to download from the VO, and were expecting the warning?

Lastly, SkyView is down at the moment due to a storage system error so I'll test your other error message once it comes back online. Thanks!

@duytnguyendtn
Copy link
Collaborator Author

Hi Cami!

Now that SkyView is back up I was able to start debugging your aforementioned issue. In short, I did get an error, but it was a download timeout error, not the join error your screenshot has. After I pushed 5a61cc9 which effectively disables the timeout, I was able to successfully download the file you specified in the screenshot. Could you try again? I'm wondering if either you ran into an issue with SkyView when we were figuring out our storage failure, or if you found a platform-specific error. I included a code snippet for reproducibility. Thanks!

vo_plugin = imviz.plugins['Virtual Observatory']._obj
vo_plugin.viewer_selected = "Manual"
vo_plugin.source = "186.70493, 21.83403"
vo_plugin.waveband_selected = "optical"
vo_plugin.vue_query_registry_resources()
vo_plugin.resource_selected = "SDSS"
vo_plugin.vue_query_resource()
vo_plugin.table.selected_rows = [vo_plugin.table.items[0]]
vo_plugin.vue_load_selected_data()

image

@duytnguyendtn
Copy link
Collaborator Author

duytnguyendtn commented Sep 12, 2024

Hi @camipacifici! I've implemented basic versions of the two requests you had:

  1. 03a00e3 implements the "recalculate center" button for the viewer coordinate select (which only shows when the user is NOT on manual mode)
  2. 783262a adds a dropdown to change the units of the search radius (Currently have degrees, radians, arcmins, and arcsecs).

NOTE: I have a pyvo PR open merged but not released to simplify the code needed in Jdaviz to enable (2), so this currently depends on the main branch. Please install main when you try it out.

image

There are still nice-to-haves I'd like to add when I have a chance, such as detecting when the coordinates pulled are out of date (probably by changing the color of the centering icon), but hopefully this increment gives you enough to try out the behavior. Let me know your thoughts!

@duytnguyendtn
Copy link
Collaborator Author

Hi @camipacifici!

Another quick update. So... I know we were concerned about the performance of auto-updating coordinates during pan-zoom, but I was curious and went ahead and implemented it anyways to see how it would perform. Surprisingly, it does quite well! Any lag I feel doesn't seem to get worse if I enable the "following" switch (alternatively, I don't see a lag improvement when I disable the toggle):

image

Please try it out and see how it performs on your side! I feel like it massively improves the user experience in searching for coordinates, but if it doesn't work for you, I can always remove it.

Thanks!
Duy

@camipacifici
Copy link
Contributor

Thank you for all the updates! Apparently, my installation was not updating and that's why I could not see the latest improvements. I now see the warning properly, the new choice for the units, and the recentering (which is surprisingly efficient!).

I went one step further and tried to load two images (using the same search of optical-CANDELS). It loaded them fine and I could link them by WCS, but I saw some weirdness when I tried to change the plot options. I now have 3 layers (A, B, C). Layer B is all black and I have not checked if the image is missing or all black in the database or something else is wrong. When I try to edit layer C, the tool keeps trying to edit layer B. I have not checked yet if this problem is also in main. Here are three screenshots:
The name of the image in layer B.
Screenshot 2024-09-16 at 2 28 51 PM
The name of the image in layer C.
Screenshot 2024-09-16 at 2 30 15 PM
The fact that C is not responding to plot options (eye says it is showing, tab says it is not).
Screenshot 2024-09-16 at 2 30 31 PM

@duytnguyendtn
Copy link
Collaborator Author

Taking a quick look at the screenshots, if the two hover texts are for the different layers, they look the same to me; it appears somehow the two copies got added with the same name some how. I'll try to reproduce and see how the data label is being applied; I could have sworn when a data label already exists, Jdaviz automatically appends a (1) to the end of it

@camipacifici
Copy link
Contributor

Careful, they are not the same. One is gsd04 and one is gsd12_sect32.

@duytnguyendtn
Copy link
Collaborator Author

Hmm, no idea then! I'll poke around and see if I find anything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imviz plugin Label for plugins common to multiple configurations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants