Closed bnmajor closed 8 months ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
💪
Let's exclude the Getter's notebook from the ones executed in CI until browser support is available.
Sounds good!
@thewtex One more thing to note: It's not a bug per-se, but if cells are run quickly or with something like Run All
the getters may return before the image data is set and the returned default value may not be useful. For example, running the following without waiting for the data to load:
viewer = view(image)
color_range = viewer.get_image_color_range()
viewer.set_image_color_range([color_range[0], color_range[1] / 2])
The last cell will actually fail because color_range
is currently None. We can account for image loading and differ execution until the data is ready (similar to what we do with waiting for the viewer to exist right now) or we can accept this behavior as-is - curious as to your thoughts?
@bnmajor thanks for the note!
Yes, I think we should wait for the image to be loaded before returning from a getter, if possible.
I know you have been iterating on the async queue handling lately, but if it is possible to add all the set's and get's to the queue and "await" all the tasks before invoking the next getter, or a similar approach, we could have "Run All" work as expected?
@bnmajor thanks for the note!
Yes, I think we should wait for the image to be loaded before returning from a getter, if possible.
I know you have been iterating on the async queue handling lately, but if it is possible to add all the set's and get's to the queue and "await" all the tasks before invoking the next getter, or a similar approach, we could have "Run All" work as expected?
I may be missing something in your question (maybe you have a specific case in mind?) but this should already be the case once the changes in this branch are in. With the exception of data not being loaded like I was explaining above, everything is essentially "set aside" until it can be run, and it should all be run in order. As an example you should be able to run the first 4 cells of the GettersAndSetters notebook and wait for the data to load, then use Run > Run Selected Cell and All Below
and everything should run as expected. Once the changes waiting for data to load are in Run All
should completely work as expected.
A few additional updates:
"tags": ["skip-execution"]
key to the cell metadata. Once testing has been fixed this tag should be removed.~A final commit is in progress to fix the compare_images
top-level wrapper. Currently:~
from itkwidgets import view
viewer = view()
viewer.compare_images(...) # succeeds
from itkwidgets import compare_images
compare_images(...) # fails
UPDATE: This is fixed now. Depends on Kitware/itk-vtk-viewer#725.
NOTE: There is still an outstanding bug that is produced when multiple viewers are created in the same notebook. References to the first viewer will fail after the second viewer is created.
viewer_1 = view(image)
viewer.set_rotate(True) # succeeds
viewer_2 = view(image)
viewer.set_rotate(False) # fails
An update on this: This is actually a standing issue and not introduced by the getters
branch as I originally thought. This can be reproduced with itkwidgets==1.0a40
which was pre-getters branch merge. As an example:
viewer = view(image, rotate=True)
viewer2 = view(image, rotate=True)
viewer.set_rotate(False)
This will fail to update the first viewer. I think that this may have slipped under the radar because unlike the getters, no exception is raised.
@thewtex @PaulHax I added support for all additional kwargs
through compare_images
on this branch since it only required some very minor changes
NOTE: As discussed in #587, testing needs some updates in order for notebooks to pass. As it stands, no viewer is actually being created during testing. This means that the queue that now waits for cell completion (including viewer creation) before progressing will cause all subsequent cells after the first viewer is created to fail. Commit 806e6e821179bc80c1a72eb21c936dc626616e5e "fixes" this by simply not running the cells after the first viewer is created. These changes will need to be reversed once testing is fixed.
NOTE: There is still an outstanding bug that is produced when multiple viewers are created in the same notebook. References to the first viewer will fail after the second viewer is created.
Fixes #466, #564, #568