Getters improvements

[bnmajor]

• Remove unused background threading code
• Make sure that new viewers are being tracked: When re-basing the old branch a kwarg was missed that prevented viewer registration and requests would not be deferred until the viewer was available and would instead produce an error.
• Make sure screenshot is output to correct cell: Run all would previously output the screenshot to the cell that it was currently on when the screenshot was taken rather than the cell that the viewer was created in.
• Do not return from setters: Prevents unnecessary information about futures from being output when a setter function is called.
• Await getter functions

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

Fixes #466, #564, #568

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[thewtex]

💪

Let's exclude the Getter's notebook from the ones executed in CI until browser support is available.

[bnmajor]

💪

Let's exclude the Getter's notebook from the ones executed in CI until browser support is available.

Sounds good!

[thewtex]

🥇

[bnmajor]

@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?

[thewtex]

@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]

@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.

[bnmajor]

A few additional updates:

• Rather than removing code from notebooks so that tests will pass add the "tags": ["skip-execution"] key to the cell metadata. Once testing has been fixed this tag should be removed.
• Cells do not progress until the data has been loaded in the viewer so that the correct information is returned from getters
• Wait for viewers to resolve before creating the next. This means that even with many viewers and run all all viewers will be created (this was an outstanding bug)

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.

[bnmajor]

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.

[bnmajor]

@thewtex @PaulHax I added support for all additional kwargs through compare_images on this branch since it only required some very minor changes

[thewtex]

-> relative import

[thewtex]

e.g.

from .viewer import Viewer

[bnmajor]

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.