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

Add table view for group peak workspace #37166

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MohamedAlmaki
Copy link
Contributor

@MohamedAlmaki MohamedAlmaki commented Apr 15, 2024

Description of work

This pull request introduces a new table view for group peaks workspaces. The new group table view will display the same data as the table view for single peak workspaces but with additional columns for the index of the peak workspace in the group and the index of the spectrum in the peak workspace. Moreover, it has the same features as the single peak workspace table view like sorting, deleting, copying, and plotting.

Fixes #36592.

Further detail of work

The work is implemented without adding a new data structure for the group peak workspace as it is not needed and all of the table view operations can be done with the existing data structure. The new view has been implemented by extending the existing table display MVP classes with new presenters and models for the group workspace.

To test:

Load and Group Workspaces:

  • Load multiple peak workspaces and verify they appear in the workspace tree.
  • Use the "GroupWorkspace" algorithm to group them.

Verify Group Data View:

  • Right-click the group in ADS and select "Show Data."
  • Verify the new table view has the same data of all the workspaces in the group.
  • The table should include "Group Index" indicating workspace position in the group and "WS Index" showing the workspace index in the corresponding peak workspace.

Interact with the Table View:

  • Test copying data (single/multiple rows, single/multiple columns) to the clipboard and paste it in another file.
  • Test deleting rows (single/multiple) and confirm successful removal.
  • Sort data by column (ascending/descending) and ensure correct sorting.
  • Select "Statistics on Columns" and verify the output includes tables with different statistics (e.g., Mean, Std Dev, Min/Max) for each column in each peak workspace
  • Select columns for 'X', 'Y', and 'Y Error'. Choose various plot types and check the data is visualized correctly.
  • Test hiding/unhiding columns

Reviewer

Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.

Code Review

  • Is the code of an acceptable quality?
  • Does the code conform to the coding standards?
  • Are the unit tests small and test the class in isolation?
  • If there is GUI work does it follow the GUI standards?
  • If there are changes in the release notes then do they describe the changes appropriately?
  • Do the release notes conform to the release notes guide?

Functional Tests

  • Do changes function as described? Add comments below that describe the tests performed?
  • Do the changes handle unexpected situations, e.g. bad input?
  • Has the relevant (user and developer) documentation been added/updated?

Does everything look good? Mark the review as Approve. A member of @mantidproject/gatekeepers will take care of it.

Gatekeeper

If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.

@MohamedAlmaki MohamedAlmaki marked this pull request as ready for review April 26, 2024 11:33
@RichardWaiteSTFC RichardWaiteSTFC self-assigned this Apr 29, 2024
Copy link
Contributor

@RichardWaiteSTFC RichardWaiteSTFC 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 this, not had chance to look through the code in detail - but have tried this out quickly. Overall it behaves well, sorting, plotting etc. just a couple of minor things:

  1. Sorting seems to work on the Qt view only - perhaps the user would expect SortPeaksWorkspace to be called on the individual tables (in common with the behaviour of other algorithms)?
  2. Deleting a row in one of the tables works even if the combined table is open, and the view is updated which indicates the ADS observer is working if an algorithm operates on one of the constituent tables. However there is a crash if you call an algorithm (e.g. FilterPeaks) replacing the group itself (i.e. InputWorkspace=OutputWorkspace="group_ws")
FilterPeaks-[Notice] FilterPeaks started
Error occurred in handler:
Traceback (most recent call last):
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\observers\ads_observer.py", line 23, in wrapper
    func(*args, **kwargs)
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\observers\ads_observer.py", line 70, in replaceHandle
    self.presenter.replace_workspace(name, workspace)
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\presenter.py", line 165, in replace_workspace
    self.presenter.model = TableWorkspaceDisplayModel(workspace)
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\model.py", line 63, in __init__
    self.supports(ws)
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\model.py", line 55, in supports
    raise ValueError("The workspace type is not supported: {0}".format(ws))
ValueError: The workspace type is not supported: peaks_out
FilterPeaks-[Notice] FilterPeaks started (child)
FilterPeaks-[Notice] FilterPeaks successful, Duration 0.00 seconds
FilterPeaks-[Notice] FilterPeaks started (child)
FilterPeaks-[Notice] FilterPeaks successful, Duration 0.00 seconds
FilterPeaks-[Notice] FilterPeaks successful, Duration 0.02 seconds. Processed as a workspace group
Python-[Error] Traceback (most recent call last):
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\table_model.py", line 127, in flags
    editable = self._data_model.is_editable_column(col)
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\group_model.py", line 107, in is_editable_column
    return self.get_column_headers()[icol] in self.EDITABLE_COLUMN_NAMES
  File "c:\mantid-conda\mantid\qt\python\mantidqt\mantidqt\widgets\workspacedisplay\table\group_model.py", line 82, in get_column_headers
    return ["Group Index", "WS Index"] + self.ws[0].getColumnNames()
RuntimeError: Variable invalidated, data has been deleted.

Python-[Fatal] Terminated by user.

It looks like FilterPeaks is run on both workspaces, it's just a invalidated variable when updating the view?
3. There is a column called WS Index - is this necessary? there is already a PeakNumber column which some algorithms (e.g. PredictPeaks) populate
4. Small thing, but if you double-click on a table workspace it opens the view, this doesn't seem to work for the group workspace, you have to right-click > Show data

@MohamedAlmaki
Copy link
Contributor Author

Thanks for the feedback.

  1. Yes, that makes sense. I thought it might be better to keep the original orderings if the user doesn't want to modify the original workspaces but I will add calls for SortPeaksWorkspace.
  2. That's right, I noticed where the problem is and will fix it.
  3. I added it for clarity and it is used by the delete action to locate the spectra in the original workspace. There is no necessary need for it so I will delete it.
  4. Ok, I will fix this.

Copy link
Contributor

@RichardWaiteSTFC RichardWaiteSTFC 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 the fixes - running FilterPeaks on the group peak workspace works now.
Just a couple new issues though!

a. Double-clicking on a group of peaks workspaces opens the table view, but now double-clicking on a single peaks workspace prints this error to the log

The workspace type is not supported: peaks_ub0
Could not open workspace: peaks_ub0 with neither MatrixWorkspaceDisplay nor TableWorkspaceDisplay.

The table view does open though!

b. There is an issue sorting the columns with the group table view open - it seems to update the view with the first table only and the column WS Index re-appears.
image

@MohamedAlmaki
Copy link
Contributor Author

Thanks for the review:
a. Ok, I will check that problem
b. I think this problem happened because you didn't scroll to the end of the workspace. the table view loads the rows dynamically so if you scroll down until the end and sort, then you will see the whole table sorted. I will look into the column reappearing problem.

@sf1919 sf1919 added this to the Release 6.10 milestone May 8, 2024
@MohamedAlmaki MohamedAlmaki added Diffraction Issues and pull requests related to diffraction ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS labels May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Diffraction Issues and pull requests related to diffraction ISIS Team: Diffraction Issue and pull requests managed by the Diffraction subteam at ISIS
Projects
Status: In review
Status: No status
Development

Successfully merging this pull request may close these issues.

Investigate supporting multiple UBs in matrix and peak workspace
4 participants