-
-
Notifications
You must be signed in to change notification settings - Fork 402
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
Implementation of ImageStack #5751
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5751 +/- ##
==========================================
- Coverage 88.35% 88.27% -0.08%
==========================================
Files 310 311 +1
Lines 63983 64340 +357
==========================================
+ Hits 56529 56798 +269
- Misses 7454 7542 +88
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e1be607
to
5aa98b6
Compare
Nice to have:
|
Does hover need support? |
Hover support is out of scope for this PR as it also needs work in Bokeh (or a new release). See bokeh/bokeh#13193, bokeh/bokeh#13200, and bokeh/bokeh#13354. |
Yeah definitely an issue but a separate issue. |
I believe this is ready for initial review, but I don't understand the invert_x/yaxis tests because although the tests pass, the inverted data matches the original data and it renders as expected. |
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.
A few comments, otherwise this looks good.
Hmm this error seems a bit tricky: Traceback```python
|
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.
I just had a quick look at the code and left some comments.
For the hover problem, try to filter the _hover_opts
method. For now, I think we should remove these extra dimensions, as we can only get one layer of information for now. When Bokeh 3.3 is released, we can extract more information because of bokeh/bokeh#13193 (maybe we can already get this with a dev release). Further down the line, we should be able to get even better support with bokeh/bokeh#13200.
(Merged in main to be able only to be able to see the changes made in this PR)
It is a limitation in Bokeh. Try to read the issues I mentioned in my last post. |
yes |
Should I be worried about the core tests? Eh I just wrapped try/except. |
I would rather see something like this: holoviews/holoviews/tests/core/data/test_binneddatasets.py Lines 225 to 228 in d3f3458
|
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.
Apart from the minor comments left by @hoxbro and I this looks great.
WIP
Reference for me:
#4806
bokeh/bokeh#12356