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

2027 adjustable ROI #2031

Merged
merged 23 commits into from
Feb 20, 2024
Merged

2027 adjustable ROI #2031

merged 23 commits into from
Feb 20, 2024

Conversation

MikeSullivan7
Copy link
Collaborator

Issue

Closes #2027.

Description

A table has been added under the ROI tabs in the Spectrum Viewer which displays the properties of the currently selected ROI (selected by clicking on the ROI Table). Currently the properties displayed are: Left, Top, Right, Bottom, Width, Height, Area.
The currently selected ROI can be adjusted by using spin boxes in the table allowing for more precise positioning.
This table also functions for the RITS ROI when that tab is activated.

More properties and details about the ROIs can be added as needed/required.

Unit Tests will be written in a future commit.

image

Testing

make check

Acceptance Criteria

Check that the numbers in the table are correct and update accordingly (i.e. when the ROI is changed).
Check that changing the numbers in the Table update the ROI properly.
The rest of the Spectrum viewer should function the same as before.

@coveralls
Copy link

coveralls commented Feb 5, 2024

Coverage Status

coverage: 74.565% (-0.7%) from 75.257%
when pulling a34a968 on 2027_adjustable_ROI
into daec4ac on main.

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

If you're happy to implement it, when an ROI is clicked in the image view, I think we should select that ROI within the ROI table as this seems like it should already be the case intuitively.

@JackEAllen
Copy link
Collaborator

Looks like Applitools tests need to be updated: https://eyes.applitools.com/app/test-results/00000251694396968241/00000251694396790667?accountId=-yE-mpxluECTdfURli4jgA~~

@JackEAllen
Copy link
Collaborator

Can you please add some release notes

@JackEAllen
Copy link
Collaborator

Can you increase the minimum height and width of the container to remove the vertical scroll on opening spectrum viewer and prevent cut-off the "Width" label
image

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Thanks for condensing. just a few little bits left now

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Looks good! just a few more tiny things and then this is good to merge

@JackEAllen
Copy link
Collaborator

I'd recommend rebasing to update you're branch with main to double check the bug isn't implemented by your work as the bug doesn't exist on main and I don't believe it is the proposed changes on this branch causing the bug, but if you remove all ROI's and swap to the image mode and the return back to the ROI mode, can your replicate experiencing a KeyError exception where the default ROI is not re-added after swapping views.?
image

@MikeSullivan7
Copy link
Collaborator Author

I'd recommend rebasing to update you're branch with main to double check the bug isn't implemented by your work as the bug doesn't exist on main and I don't believe it is the proposed changes on this branch causing the bug, but if you remove all ROI's and swap to the image mode and the return back to the ROI mode, can your replicate experiencing a KeyError exception where the default ROI is not re-added after swapping views.? image

This bug should now be fixed now, I changed the table behaviour slightly when the tabs are switched back and forth even with all ROIs removed. I do not see this error when testing so can you please check whether you still see this bug?

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the changes in! Looks really good!
Just two small things now and we're there! 👍

If I delete all ROIs from the ROI view, then navigate to the Image view, the spin boxes need to be re-enabled for that view then disabled again on returning to Image view if no ROI's exist (See screenshot)

image

If i remove all ROI's from ROI view, navigate to Image view, swap samples, then return to ROI view, the state changes are not synced up causing the KeyError (see screenshot and logs)

image

Logs:

2024-02-16 13:59:44,642 [mantidimaging.core.utility.progress_reporting.progress:L241] INFO: Elapsed time: 11 sec.
2024-02-16 14:00:00,263 [perf.mantidimaging.gui.mvp_base.view:L59] INFO: SpectrumViewerWindowView shown in 0.8960616621188819
2024-02-16 14:00:10,615 [py.warnings:L109] WARNING: /home/vco13151/mambaforge/envs/mantidimaging-dev/lib/python3.10/site-packages/pyqtgraph/graphicsItems/PlotItem/PlotItem.py:509: UserWarning: Item already added to PlotItem, ignoring.

2024-02-16 14:00:13,657 [mantidimaging.gui.windows.main.view:L513] ERROR: Traceback (most recent call last):
  File "/home/vco13151/mantidimaging/mantidimaging/gui/windows/spectrum_viewer/presenter.py", line 288, in handle_export_tab_change
    self.view.on_visibility_change()
  File "/home/vco13151/mantidimaging/mantidimaging/gui/windows/spectrum_viewer/view.py", line 221, in on_visibility_change
    self.set_roi_properties()
  File "/home/vco13151/mantidimaging/mantidimaging/gui/windows/spectrum_viewer/view.py", line 450, in set_roi_properties
    current_roi = self.presenter.model.get_roi(self.current_roi)
  File "/home/vco13151/mantidimaging/mantidimaging/gui/windows/spectrum_viewer/model.py", line 122, in get_roi
    raise KeyError(f"ROI {roi_name} does not exist in roi_ranges {self._roi_ranges.keys()}")
KeyError: "ROI None does not exist in roi_ranges dict_keys(['all', 'roi', 'rits_roi'])"

2024-02-16 14:00:13,657 [mantidimaging.gui.mvp_base.view:L40] ERROR: show_error_dialog(): Uncaught exception KeyError: "ROI None does not exist in roi_ranges dict_keys(['all', 'roi', 'rits_roi'])"

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Just this to fix now and good to merge

Copy link
Collaborator

@JackEAllen JackEAllen left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making the changes

@JackEAllen JackEAllen added this pull request to the merge queue Feb 20, 2024
Merged via the queue into main with commit e9df08c Feb 20, 2024
8 checks passed
@JackEAllen JackEAllen deleted the 2027_adjustable_ROI branch February 20, 2024 13:49
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.

Adjustable ROI in Spectrum Viewer
4 participants