Open
Conversation
…ove workspaces names
- Modify how data model sets and retrieves resolutions - Add support for new fitdata structure with resolution map
- Remove observer inheritance
…me and delete handles - Add notifier functions using Qmetaobject to properly handle threads
…ution at the end so that model is set with all appropriate paramters
…odel function signature that includes the resolution spectra - add table entry without retrieving all resolutions
- Refactor a bit to remove redundancy - Add need tests covering new way of handling resolution in model
adriazalvarez
added
Bug
adriazalvarez
changed the title
Contributor
Unit test results2 863 tests 2 863 ✅ 8h 20m 43s ⏱️ Results for commit 51eb826. ♻️ This comment has been updated with latest results. |
Contributor
System test results808 tests 792 ✅ 3h 1m 5s ⏱️ Results for commit 51eb826. ♻️ This comment has been updated with latest results. |
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR refactors workspace lifecycle management in QENS fitting to prevent crashes when workspaces are removed from the Analysis Data Service. It relocates ADS observation from presenters to the view layer, allowing the view to forward ADS events (delete, rename, clear) to presenters. The DataModel resolution API is expanded from simple name-based methods to context-aware methods accepting workspace names and spectra indices. Internal resolution tracking shifts from weak workspace pointers to per-workspace spectrum-indexed mappings within Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (6)
qt/widgets/spectroscopy/test/DataModelTest.h (1)
257-266: Clarify the "no workspaces without resolution" invariant.The comment on line 264 states "There can't be workspaces without resolution", and the test verifies that removing all resolutions also clears all workspaces. This is an important design invariant for the convolution fitting workflow. Ensure this behavior is documented in the
DataModelclass orremoveResolutionmethod documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/widgets/spectroscopy/test/DataModelTest.h` around lines 257 - 266, Document the invariant that workspaces cannot exist without an associated resolution in the DataModel API: update the DataModel class description and the removeResolution(...) method docstring to state that removing all resolutions will clear workspaceNames (see methods resolutionNames(), workspaceNames(), updateWorkspaceNames()) and that removeResolution triggers this cleanup, referencing the behavior tested in test_resolutions_reference. Make the doc change concise, state the expected post-condition (workspaceNames empty if no resolutions), and mention callers should not assume workspaces persist after removals.qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/IDataModel.h (1)
72-73: Consider clarifying the distinction betweengetResolutionNames()andresolutionNames().The interface has two methods with similar names but different semantics:
getResolutionNames()(line 73): non-const, returnsstd::set<std::string>by valueresolutionNames()(line 87): const, returnsconst std::set<std::string>&If
getResolutionNames()computes/filters the names whileresolutionNames()returns a cached set, consider renaming for clarity (e.g.,computeResolutionNames()vsresolutionNames()), or add documentation to clarify the distinction.Also applies to: 87-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/IDataModel.h` around lines 72 - 73, The two similarly named methods getResolutionNames() and resolutionNames() are confusing because getResolutionNames() is non-const and returns a set by value while resolutionNames() is const and returns a const reference; update the API to make the distinction explicit by either renaming getResolutionNames() to something like computeResolutionNames() or filteredResolutionNames() if it performs computation/filtering, or by adding clear doc-comments above both declarations describing their semantics (e.g., "compute and return a new set of resolution names" vs "return cached resolution names by reference") and ensure the chosen names/comments are applied consistently for the pair resolutionNames() and getResolutionNames().qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp (1)
54-64: Consider removingconstqualifier fromremoveADSWorkspace.This method is marked
constbut has observable side effects (removing workspaces and resolutions from the model). While technically valid C++ (theconstapplies tothis, notm_model), it can be misleading to readers. Consider removing theconstqualifier to better reflect the method's behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp` around lines 54 - 64, The method FitDataPresenter::removeADSWorkspace is declared const but mutates the model via m_model->removeWorkspaceByName and m_model->removeResolution; remove the const qualifier from the method signature so its declaration and definition reflect that it has side effects (update the signature of removeADSWorkspace to be non-const wherever declared/defined, keeping the existing logic and return value), and ensure any callers or overrides compile after the change.qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/DataModel.h (1)
73-90: Consider clarifying the difference betweengetResolutionNames()andresolutionNames().There are two methods for obtaining resolution names:
getResolutionNames()(line 77): Returns a freshly computedstd::set<std::string>by valueresolutionNames()(line 89): Returnsconst std::set<std::string>&to the cachedm_uniqueResWsNamesThis dual interface could lead to confusion. Consider either:
- Adding documentation to clarify when to use each
- Renaming one for clarity (e.g.,
computeResolutionNames()vscachedResolutionNames())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/DataModel.h` around lines 73 - 90, The two similarly named APIs getResolutionNames() and resolutionNames() are confusing because one returns a computed set by value and the other returns a const reference to the cached m_uniqueResWsNames; rename them to make intent explicit (e.g., rename getResolutionNames() -> computeResolutionNames() and resolutionNames() -> cachedResolutionNames()) and update all usages, or alternatively add clear doc-comments above both functions explaining which returns a fresh set and which returns the cached reference and reference m_uniqueResWsNames; ensure any public/consumer call sites are updated (or provide a short deprecated wrapper if you must preserve the old names).qt/widgets/spectroscopy/src/DataModel.cpp (2)
112-129: Consider simplifying the resolution lookup logic.The nested conditionals and map lookups make this logic hard to follow. Consider extracting the resolution lookup into a helper function for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/widgets/spectroscopy/src/DataModel.cpp` around lines 112 - 129, Refactor DataModel::getResolutionsForFit by extracting the resolution lookup and workspace-check logic into a private helper (e.g., resolveResolutionInfo or isSingleHistogramResolution) that takes a const std::string& resName and returns a pair<bool /*exists*/, bool /*singleHistogram*/>, then replace the inline checkedResolutions handling inside the loop with calls to that helper and cache results in a local unordered_map<string,bool> if needed; use m_adsInstance.doesExist and m_adsInstance.retrieveWS<Mantid::API::MatrixWorkspace> inside the helper and keep the outer loop only responsible for building resolutionVector from the helper’s result and the incoming spectrum.value.
51-56: Document or handlegetWorkspaceIDreturning an out-of-bounds value.When the workspace name is not found,
getWorkspaceIDreturns aWorkspaceIDwith value equal to the container size (out-of-bounds). This is consistent with STL iterator semantics but could lead to subtle bugs if callers don't validate the result. Consider either:
- Adding a comment documenting this behavior
- Returning
std::optional<WorkspaceID>to make the "not found" case explicitThe test at
DataModelTest.h:81documents this behavior, but the implementation itself has no indication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qt/widgets/spectroscopy/src/DataModel.cpp` around lines 51 - 56, The getWorkspaceID method currently returns a WorkspaceID constructed from (iter - names.cbegin()) which yields names.size() when not found; update the implementation to make the "not found" case explicit: either add a clear comment above DataModel::getWorkspaceID documenting that it returns a sentinel index equal to container size when the name is missing, or change the API to return std::optional<WorkspaceID> (adjusting callers and tests such as DataModelTest to handle std::nullopt), i.e., detect if iter == names.cend() and then return std::nullopt (or the documented sentinel) so callers cannot silently use an out-of-bounds ID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp`:
- Around line 31-39: In FitDataPresenter::handleADSRename the concatenation for
the variable names produces a leading " , " when removedNew is false and
removedOld is true; change the construction to only insert the separator when
names is non-empty (e.g. build a small list of removed names or append with a
conditional that checks if names.empty() before adding " , "), leaving the
existing removedNew/removedOld checks, updateTab() call and the final
displayWarning("Data " + names + " was removed from the ADS.") intact so the
message never starts with a stray separator.
- Around line 270-273: Remove the temporary debug printing in
FitDataPresenter.cpp: delete the std::cout line inside the loop that iterates
over selectedIndices (the block using m_model->getSubIndices(index.row()) and
the variables wsId and wsIndex). Ensure no other leftover console/debug prints
remain in that loop; if needed, replace with appropriate logging calls or
silently proceed with the loop logic that follows.
In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.h`:
- Around line 34-36: The parameter order for handleADSRename is inconsistent:
IFitDataPresenter declares (const std::string &newName, const std::string
&oldName) but FitDataPresenter overrides handleADSRename(const std::string
&oldName, const std::string &newName). Change the IFitDataPresenter declaration
to match the ADS convention and the override by declaring virtual void
handleADSRename(const std::string &oldName, const std::string &newName) = 0; and
update any callers/implementations to use the (oldName, newName) ordering so all
declarations and overrides (IFitDataPresenter::handleADSRename and
FitDataPresenter::handleADSRename) are consistent.
In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataView.cpp`:
- Around line 61-64: The rename parameter order is inverted:
FitDataView::renameHandle currently passes (newName, oldName) into
notifyADSRename which ultimately calls m_presenter->handleADSRename(newName,
oldName), while FitDataPresenter::handleADSRename is declared as (const
std::string &oldName, const std::string &newName) (and the base
AnalysisDataServiceObserver::renameHandle uses (wsName, newName) where wsName is
the old name). Fix by aligning parameter order—either change
FitDataView::renameHandle to invoke notifyADSRename with (oldName, newName) or
swap the arguments when calling m_presenter->handleADSRename inside
notifyADSRename so the presenter receives (oldName, newName); ensure the
signature of FitDataPresenter::handleADSRename remains (oldName, newName) to
match the base class.
In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataView.h`:
- Around line 50-52: The override signature for renameHandle in FitDataView.h
uses parameter names newName and oldName but the base class
AnalysisDataServiceObserver defines renameHandle(const std::string &wsName,
const std::string &newName) where wsName is the old name; fix the header to
match the base by changing the parameter order/names to (const std::string
&oldName, const std::string &newName) or better (const std::string &wsName,
const std::string &newName) and then update the implementation in
FitDataView.cpp (the renameHandle method) to use the same parameter names and
semantics so the old workspace name is the first argument and the new name is
the second.
In
`@qt/scientific_interfaces/Inelastic/QENSFitting/InelasticFitPropertyBrowser.cpp`:
- Around line 428-439: The try block can leave the two browser presenters in a
partially-updated state because resetLocalParameters() is a no-op; save the
previous state for both m_functionBrowser and m_templatePresenter (e.g.,
previous nData/datasets/qValues/resolutions), then perform the updates
(setNumberOfDatasets, setDatasets, setQValues, setResolution) and if any
exception is thrown restore the saved state for both presenters (or clear them
to a known empty state) so the UI never remains partially updated; alternatively
make the update atomic by preparing all new data off-line and only calling the
setters once when ready, and ensure you call a meaningful recovery method on
both m_functionBrowser and m_templatePresenter (not just resetLocalParameters())
when catching Mantid::Kernel::Exception::NotFoundError.
In
`@qt/scientific_interfaces/Inelastic/test/QENSFitting/ConvolutionDataPresenterTest.h`:
- Around line 96-112: The tests test_setResolution_calls_to_model and
test_setResolution_has_bad_values use the workspace name "TestWS" but setUp()
registers "TestWs", causing mock string-matching failures; update the calls to
m_presenter->setResolution and the corresponding EXPECT_CALL/ON_CALL usages to
use the exact workspace name "TestWs" (i.e. change "TestWS" to "TestWs") so
m_model, m_view, removeSpecialValues, and displayWarning expectations match the
registered workspace name.
In `@qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/FitData.h`:
- Around line 57-60: FitData::combine() currently merges spectra, ranges and
exclude regions but neglects m_resolutions so incoming resolution assignments
are lost; update FitData::combine() to also merge other.m_resolutions into
this->m_resolutions (iterate other.m_resolutions and insert/assign each pair
into m_resolutions), choosing a conflict rule (e.g. overwrite existing entries
with the incoming value or skip if you want to keep originals) so
getResolutions() / getResolutionFromWsIndex() see the combined data; ensure any
related helper methods (getResolutions, getResolutionFromWsIndex,
removeResolutionEntry) remain consistent with the merged state.
In `@qt/widgets/spectroscopy/src/DataModel.cpp`:
- Around line 140-159: In DataModel::setResolution add a bounds check for
workspaceID before calling m_fittingData->at(workspaceID.value): validate that
workspaceID.value is within m_fittingData->size() (or the same guard logic used
in getWorkspace) and throw an appropriate std::out_of_range or
std::runtime_error with a clear message when invalid; only proceed to read the
resolution and call m_fittingData->at(...).setResolution(resName, spectra) after
the check to prevent out-of-range access.
- Around line 135-138: DataModel::setResolution(const std::string&, const
std::string&, const FunctionModelSpectra&) currently calls
getWorkspaceID(wsName) and delegates to the overload without checking the
returned workspace ID; since getWorkspaceID can return an invalid/out-of-range
value, first validate the workspace exists (e.g. check the optional/flag or
bounds returned by getWorkspaceID) before calling the workspaceID overload, and
if invalid return an error/false (or handle appropriately) instead of delegating
and risking access to m_fittingData in the workspaceID overload.
In `@qt/widgets/spectroscopy/src/FitData.cpp`:
- Around line 343-353: The membership check in FitData::setResolution currently
uses m_ranges but should use the canonical spectra container m_spectra (and the
overload calling setResolution for each spectrum) because setSpectra() updates
m_spectra not m_ranges; change the conditional in
FitData::setResolution(std::string const &wsName, WorkspaceIndex const
&spectrum) to test m_spectra.contains(spectrum) (or the appropriate lookup into
m_spectra) so added/removed spectra are respected, and ensure the batch overload
still delegates to that corrected single-spectrum setter.
---
Nitpick comments:
In `@qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp`:
- Around line 54-64: The method FitDataPresenter::removeADSWorkspace is declared
const but mutates the model via m_model->removeWorkspaceByName and
m_model->removeResolution; remove the const qualifier from the method signature
so its declaration and definition reflect that it has side effects (update the
signature of removeADSWorkspace to be non-const wherever declared/defined,
keeping the existing logic and return value), and ensure any callers or
overrides compile after the change.
In `@qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/DataModel.h`:
- Around line 73-90: The two similarly named APIs getResolutionNames() and
resolutionNames() are confusing because one returns a computed set by value and
the other returns a const reference to the cached m_uniqueResWsNames; rename
them to make intent explicit (e.g., rename getResolutionNames() ->
computeResolutionNames() and resolutionNames() -> cachedResolutionNames()) and
update all usages, or alternatively add clear doc-comments above both functions
explaining which returns a fresh set and which returns the cached reference and
reference m_uniqueResWsNames; ensure any public/consumer call sites are updated
(or provide a short deprecated wrapper if you must preserve the old names).
In `@qt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/IDataModel.h`:
- Around line 72-73: The two similarly named methods getResolutionNames() and
resolutionNames() are confusing because getResolutionNames() is non-const and
returns a set by value while resolutionNames() is const and returns a const
reference; update the API to make the distinction explicit by either renaming
getResolutionNames() to something like computeResolutionNames() or
filteredResolutionNames() if it performs computation/filtering, or by adding
clear doc-comments above both declarations describing their semantics (e.g.,
"compute and return a new set of resolution names" vs "return cached resolution
names by reference") and ensure the chosen names/comments are applied
consistently for the pair resolutionNames() and getResolutionNames().
In `@qt/widgets/spectroscopy/src/DataModel.cpp`:
- Around line 112-129: Refactor DataModel::getResolutionsForFit by extracting
the resolution lookup and workspace-check logic into a private helper (e.g.,
resolveResolutionInfo or isSingleHistogramResolution) that takes a const
std::string& resName and returns a pair<bool /*exists*/, bool
/*singleHistogram*/>, then replace the inline checkedResolutions handling inside
the loop with calls to that helper and cache results in a local
unordered_map<string,bool> if needed; use m_adsInstance.doesExist and
m_adsInstance.retrieveWS<Mantid::API::MatrixWorkspace> inside the helper and
keep the outer loop only responsible for building resolutionVector from the
helper’s result and the incoming spectrum.value.
- Around line 51-56: The getWorkspaceID method currently returns a WorkspaceID
constructed from (iter - names.cbegin()) which yields names.size() when not
found; update the implementation to make the "not found" case explicit: either
add a clear comment above DataModel::getWorkspaceID documenting that it returns
a sentinel index equal to container size when the name is missing, or change the
API to return std::optional<WorkspaceID> (adjusting callers and tests such as
DataModelTest to handle std::nullopt), i.e., detect if iter == names.cend() and
then return std::nullopt (or the documented sentinel) so callers cannot silently
use an out-of-bounds ID.
In `@qt/widgets/spectroscopy/test/DataModelTest.h`:
- Around line 257-266: Document the invariant that workspaces cannot exist
without an associated resolution in the DataModel API: update the DataModel
class description and the removeResolution(...) method docstring to state that
removing all resolutions will clear workspaceNames (see methods
resolutionNames(), workspaceNames(), updateWorkspaceNames()) and that
removeResolution triggers this cleanup, referencing the behavior tested in
test_resolutions_reference. Make the doc change concise, state the expected
post-condition (workspaceNames empty if no resolutions), and mention callers
should not assume workspaces persist after removals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d94fd236-141c-470c-85ff-74349ae469d7
📒 Files selected for processing (20)
qt/scientific_interfaces/Inelastic/QENSFitting/ConvolutionDataPresenter.cppqt/scientific_interfaces/Inelastic/QENSFitting/ConvolutionDataPresenter.hqt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cppqt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.hqt/scientific_interfaces/Inelastic/QENSFitting/FitDataView.cppqt/scientific_interfaces/Inelastic/QENSFitting/FitDataView.hqt/scientific_interfaces/Inelastic/QENSFitting/FittingModel.cppqt/scientific_interfaces/Inelastic/QENSFitting/FunctionQDataPresenter.hqt/scientific_interfaces/Inelastic/QENSFitting/InelasticFitPropertyBrowser.cppqt/scientific_interfaces/Inelastic/test/QENSFitting/ConvolutionDataPresenterTest.hqt/scientific_interfaces/Inelastic/test/QENSFitting/FitDataPresenterTest.hqt/widgets/common/src/ConvolutionFunctionModel.cppqt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/DataModel.hqt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/FitData.hqt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/IDataModel.hqt/widgets/spectroscopy/inc/MantidQtWidgets/Spectroscopy/MockObjects.hqt/widgets/spectroscopy/src/DataModel.cppqt/widgets/spectroscopy/src/FitData.cppqt/widgets/spectroscopy/test/DataModelTest.hqt/widgets/spectroscopy/test/FitDataTest.h
qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp
Outdated
Show resolved
Hide resolved
qt/scientific_interfaces/Inelastic/QENSFitting/FitDataPresenter.cpp
Outdated
Show resolved
Hide resolved
qt/scientific_interfaces/Inelastic/test/QENSFitting/ConvolutionDataPresenterTest.h
Show resolved
Hide resolved
- Main change was some name inconsistencies between old and new name when overriding handleRename from the observers
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of work
This issue closes #40731 , #41020. Origin of both crashes are two aspects of the same feature: the management of resolution workspaces from the DataModel. The resolution workspaces are stored as a vector of weak pointers to resolution workspaces loaded on the ADS. When data from the ADS revolving these workspaces is removed, the function browser tries to access a pointer that's been voided. Additionally, there was a problem when setting the spectra for resolutions when new spectra were added to the interface, this made impossible to add new spectra.
The fix this issue, there had been two main changes:
Closes #40731 , #41020 .
To test:
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
mantid-developersormantid-contributorsteams, add a review commentrerun cito authorize/rerun the CIGatekeeper
As per the gatekeeping guidelines: