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

Fix logic and descriptions of set_channel and set_polarization. #101

Merged
merged 1 commit into from
Apr 14, 2023

Conversation

confluence
Copy link
Collaborator

I found a bug in this code while testing something else. The default value I was using for the polarization in set_channel returned a null value if the image had no polarizations. This should now be using the same defaults as the animator widgets. I am also using macros to avoid an unnecessary round trip to fetch the values from the frontend and send them back. Additionally, I have fixed a bug in the image class's macro method, which did not correctly handle an empty string target parameter.

I have tested this locally and as far as I can see this correctly sets the channel and polarization independently of each other.

@kswang1029
Copy link
Contributor

kswang1029 commented Apr 14, 2023

@confluence while testing this PR, I found a few usability issues. For example, in the following snippet

img0 = session.open_image(image_file_7)
img1 = session.append_image(image_file_1)

img0.make_active() # issue1
img0.set_channel(20)
img0.set_polarization(Polarization.Q)

time.sleep(5) # issue2

img1.make_active()
img1.set_channel(20)

Issue1
In this case, img1 will be the active image. Then if I do img0.set_channel(20) and/or img0.set_polarization(Polarization.Q) without img0.make_active(), we will see indeed in the image list widget, the channel index and polarization string are updated. However, the rendered image is not. We need to do img0.make_active() first to re-render the raster image. I suggest in set_channel and set_polarization we internally make the target image as "active" before setting channel/polarization. In the scripting interface when there are multiple image loaded, it is non-trivial which one is currently "active" I suppose.

Issue2
In the above snippet, if I comment out time.sleep(5), even if we do img0.make_active() first before setting channel and polarization, we do not see img0 got re-rendered. I suspect this is an sync/async issue.

Please let me know if you cannot reproduce the issue or if you have further questions. Do you think these can be addressed in the same PR (ie the logic 😉)?

@confluence
Copy link
Collaborator Author

@kswang1029 I can reproduce this, but also in dev -- this is unrelated to the function logic, and related to the inactive image not being re-rendered if this is changed. I think that we should create a new issue for this -- ideally I would prefer to solve it without silently changing the active image, which may not be what the user expects. Even if we immediately change it back, there may be a race condition (as you have found in the second issue). There may be a different way to trigger the re-rendering, or it may be possible to add one.

@kswang1029
Copy link
Contributor

@kswang1029 I can reproduce this, but also in dev -- this is unrelated to the function logic, and related to the inactive image not being re-rendered if this is changed. I think that we should create a new issue for this -- ideally I would prefer to solve it without silently changing the active image, which may not be what the user expects. Even if we immediately change it back, there may be a race condition (as you have found in the second issue). There may be a different way to trigger the re-rendering, or it may be possible to add one.

ok filed as #103

@confluence confluence merged commit 24e4ec0 into dev Apr 14, 2023
@confluence confluence deleted the confluence/set_channel_polarization_fix branch April 14, 2023 11:16
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