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

TST: See if percentile is breaking devdeps (try 2) #96

Closed
wants to merge 12 commits into from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Mar 12, 2024

Builds on top of #68 to investigate #93 . Supersedes #95 .

@pllim

This comment was marked as resolved.

@pllim

This comment was marked as resolved.

@@ -15,7 +15,8 @@
from jdaviz.configs.default.plugins.viewers import JdavizViewerMixin
from jdaviz.configs.specviz.plugins.viewers import SpecvizProfileView

from lcviz.state import ScatterViewerState
#from lcviz.state import ScatterViewerState
from glue.viewers.scatter.state import ScatterViewerState
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what actually fixed the failures, so I convinced myself now that the incompatibility is within lcviz and not jdaviz. However, in doing this, some results in test_plugin_markers.py changed and I am not sure why.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we want the lcviz overrides for resetting limits though. But something in there must not be playing nicely with the upstream PR in jdaviz that introduced the zoom-radius for Imviz 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, pretty much. There is nothing to do over at Jdaviz for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure that is the case yet, we still need to understand why ScatterViewerState in lcviz is conflicting with the logic from spacetelescope/jdaviz#2649. The fix may be here or something may need to be generalized there more, I'm not sure yet. But I think we can now be fairly certain that the configs in jdaviz are not affected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I am the best person to investigate more. I never worked on lcviz so I am not privy on what your scatter state customization is supposed to do.

@kecnry
Copy link
Member

kecnry commented Mar 29, 2024

Thanks for helping to track this down! The underlying bug was fixed by kecnry#8 for jdaviz 3.9 and #102 for lcviz main (to avoid testing with jdaviz 3.9 in devdeps)

@kecnry kecnry closed this Mar 29, 2024
@pllim pllim deleted the fix-tst-devdeps-maybe-try2 branch March 29, 2024 19:24
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