Parial move of reflectometry gui to std::optional #38545
Conversation
This file contains 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
peterfpeterson
force-pushed
the
37875_boost_optional
branch
2 times, most recently
from
821ec70 to
f121bc3
Compare
peterfpeterson
added
Reflectometry
peterfpeterson
force-pushed
the
37875_boost_optional
branch
6 times, most recently
from
a11db32 to
640da16
Compare
peterfpeterson
force-pushed
the
37875_boost_optional
branch
from
640da16 to
9a37f10
Compare
peterfpeterson
force-pushed
the
37875_boost_optional
branch
2 times, most recently
from
6aaba92 to
2b2d1a2
Compare
rbauststfc
reviewed
Jan 13, 2025
There was a problem hiding this comment.
Thanks very much for picking this one up for us! I've not done any functional testing yet, but the code changes are looking good to me and I can't see anything obvious that should be a problem (particularly given that all the tests are passing).
There was just one small thing I had a question about before I start testing the GUI.
peterfpeterson
force-pushed
the
37875_boost_optional
branch
from
2b2d1a2 to
8ae0880
Compare
|
Sorry for the delay on the functional testing. I've tested as many of the GUI features as I can, including reducing data manually and using the auto-processing feature, and it's all looking good to me. I can't check live data properly because we're out of cycle, but I've clicked the button to start it up and it displays the message we would expect to see at the moment. I'm sure there won't be any issues with it given the nature of these changes, and we'll still have a chance to test it after this has gone into main. Thanks again for picking this up for us @peterfpeterson! I will hold off clicking approve until after code freeze to make sure it isn't merged in too early. @MialLewis, just tagging you in this so that you're aware of it and the wider issue that it's linked to, as there will be more work needed on this in future. |
github-actions
bot
added
the
Has Conflicts
|
👋 Hi, @peterfpeterson, Conflicts have been detected against the base branch. Please rebase your branch against the base branch. |
peterfpeterson
force-pushed
the
37875_boost_optional
branch
from
8ae0880 to
47acae2
Compare
updates: - [github.com/astral-sh/ruff-pre-commit: v0.8.6 → v0.9.1](astral-sh/ruff-pre-commit@v0.8.6...v0.9.1)
…-ruff-ornlnext pre-commit ruff update - ornl-next
This is a squashed version of mantidproject#38367 Added release note
…rrection Algorithm This is a squashed version of mantidproject#37672 - Added spin state order handling to PolarizationCorrectionFredrikze algorithm - Implemented InputSpinStateOrder and OutputSpinStateOrder properties. - Updated execPA functions to dynamically handle different spin state orders. - Extracted input and output spin state mapping logic into separate functions for better code organization and reusability. - Ensured backward compatibility by setting default spin state orders. - Added validation to ensure input workspace count matches the spin state entered - Added spin state order handling to PolarizationCorrectionFredrikze algorithm - Implemented InputSpinStateOrder and OutputSpinStateOrder properties. - Updated execPA functions to dynamically handle different spin state orders. - Extracted input and output spin state mapping logic into separate functions for better code organization and reusability. - Ensured backward compatibility by setting default spin state orders. - Added validation to ensure input workspace count matches the spin state entered - Included unit tests to verify correct functionality and validation of spin state orders. - splitted the spin state order string to a string of vectors using the polarization correction helper splitSpinStateString added p and a to spin states - modified unit tests to test spin state orders - updated the documentation to describe the spin states and its usage Add Input and Output SpinStates for FredrikzeAlgorithm in PolarizationEfficiencyCor - added input and output spin states for fredrikze algorithm in polarizationefficiencycor algorithm - updated unit tests to test spin states in both algorithms - updated polarizationefficiencycor documentation - added a release note for the update Refactor Fredrikze Algorithm SpinStates Implementation - Renamed the input and output spin states - refactored tests to eliminate code duplication - added an helper function to fix crash which happens when user enter pp, aa as spinstates - improved code based on PR review suggestions - updated unit tests to handle crash which happens when user enter pp, aa as spinstates - updated documentation --amend-no-edit [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
This is a squashed version of mantidproject#38400 Move the PNR_LABEL and PA_LABEL constants out of the Prop namespace Rename some constants to use mantid naming convention updated documentation add static keyword to constants
…ate in log This is a squashed version of mantidproject#38322 Add unit tests to check for spin state sample log Add ORSO polarizations constants and helper methods Add optional ORSO spin state log to corrected workspaces Use static const for polarisation constants We were previously using constexpr however it seems that this might not be ideal for string constants because they cannot be passed by reference. Refactor setting sample log on output ws Refactor the code for setting the spin state sample log on the output workspace to reduce code duplication. Use spin state log constant in Wildes tests Expose AddSpinStateToLog parameter in PolarizationEfficiencyCor Update algorithm documentation Add release notes for issue 38264 [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Fix build errors and warnings Add a missing import and remove an unused variable.
This is a squashed version of mantidproject#38205 remove qobject dependency from baselinemodelling model and presenter fix cppcheck shadow variable change some model methods to const use unique pointer to baselinefitting presenter in the alc interface Fix problem where updates weren't being called after fit add IALCBaselineModellingPresenter add virtual destructors to interfaces to help test cleanup
This is a squashed version of mantidproject#38317 Put presenter calls inside view slots Instead of the previous calls: view -> emit signal -> presenter catches signal -> presenter slot Now we have: view -> emit signal -> view slot -> presenter handle This makes the code simpler and avoids the presenter being a QObject Replace signal by calling presenter directly Option to toggle auto add would trigger a signal that would call the function startWatching in the presenter. Replaced this by directly calling the function from the view, since now the presenter is accessible from the view. Remove signal from presenter Signal dataChanged() from presenter was changed to signal emmited from the view. Make presenter a non QObject Removed inheritance of presenter as a QObject. Instead of using QObject::timerEvent(), replaced it by using QTimer. Replaced connect() with QObject::connect() and replaced QObject slots with lambdas, which is possible since qt5. Removed unused slots and signals Cleaned slots and signals that were now unused due to work from previous commits. Realised that one of the slots called instrumentChanged() should be used instead of creating a new one that notifies the presenter. Corrected this oversight. Create abstract class IALCDataLoadingPresenter Created virtual methods for the public methods in the ALCDataLoadingPresenter class. Changed arguments to use the abstract class. Add destructor to virtual class Pass opening of manage directories to view Because it contains qt code and is very straight forward. Move watcher and timer into view Previuosly the watcher and timer in the presenter were QObjects. To make the presenter not dependent on qt I moved them into the view. Move period info interface to view View now owns the period info interface, so that presenter is not dependent on qt anymore. Moved watcher and timer logic to presenter Realised that logic could be moved to presenter by introducing getter methods from the view. Create model Added source and header files for model. Passed load() function to model since that is where a lot of the calculations happen. Pass some variables to model fully Put code to do with some variables that store the state whithin model fully and deleted those variables from the presenter Pass updateAvailableInfo() to model Put all of the logic to update information on logs and periods into the model. Pass extractRunNumber() into model Pass getPathFromFiles to model Pass checkGroupingValid() to model Finish separation between model and presenter This commit finishes the separation between presenter and model, by passing the function timerEvent() to the model. Now the presenter doesn't have any state variables and holds only the view and the model. Change variable name and less arguments for load() Fix seg fault crash Previously the ALC interface was crashing Mantid when it was closed. This was due to two pointers (m_watcher and m_timer) being deleted twice. I am not sure what was happening but I reckon that since they were owned by the loading interface, upon closing the ALC interface they were being deleted by the loading interface and the ALC (parent) interface both, causing a double deletion error. I fixed it by passing ownership of the wather and timere to the ALC (parent) interface. Fix cppcheck errors Fix unmatched declaration Clean header file and function declarations Added const and static to function declarations. Add abstract class for model Added abstract class for model, changed the constructor of presenter to include this abstract class and cleaned ALCInterface.cpp where the presenter is first initiated. Correct unit test of presenter Modified file for testing presenter according to the changes I made before. Fix cpp check error Was missing an override for the model destructor Make view inherit QObject instead of abstract parent Instead of abstract class of view inheriting qobject, made class of view a qobject. Simplifies signals and slots. Put function where it's used Make abstract class NOT inherit from QObject Set non-owning pointer for model inside test Useful in case it gets used in the future Add const in several functions Make getters const Refactor checkCustomGrouping into return early style Retain the same functunality but improve readability by returning early. Refactor loading files into return early style Made function easier to read. Update connections style Using the new style allows for the compiler to detect missing connections during compilation, rather than at run time. Call inputs as arguments instead of passing view Instead of passing the view to the model, it's best to pass the view inputs as arguments, so that the model never has access to the view. Rename loading function Made it clearer that load function in presenter triggers loading of files that are *already present in the model*, so that in order to load files they have to be loaded into the model first. Fix unit test Since view inputs are called as arguments of load function, neeeded to change the number of times one of the arguments was called. Correct comment Add check for forward grouping
This is a squashed version of mantidproject#38211 Remove Qt elements from model RE mantidproject#38007 Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]> Co-authored-by: James Crake-Merani <[email protected]> Fix model tests RE mantidproject#38007 Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]> Strip Qt from presenter RE mantidproject#38007 Co-authored-by: Waruna Priyankara J A Wickramasingha <[email protected]> Remove commented methods RE mantidproject#38007 Use mondern mocking standard RE mantidproject#38007 Fix CppCheck issues RE mantidproject#38007 Add missing virtual destructor RE mantidproject#38007 Update fitting tab from baseline tab RE mantidproject#38007 Add subscriber in tests RE mantidproject#38007 Remove unnecessary comments and includes RE mantidproject#38007 Modernise connections and pointers RE mantidproject#38007 [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci
This is a squashed version of mantidproject#38398 Ability to import external data into Muon ALC interface Add release note Remove signal from ALCDataHandlingView Add unit test for calling subscriber method Rename subscribe method to setSubscriber
… Algorithm This is a squashed version of mantidproject#38447 - added function to add spinstate to polarizationcorrection fredrikze algorithm - modified mapoutputworkspacetospinstate to add outspinstate to samplelog if requested - updated unit tests to check that the order of output workspaces matches the order of specified output spinstates in the samplelog - updated documentation Remove unused OutputWorkspaceName parameter from execPA and execPNR Update Test based on suggested changes from code - remove redundant unit tests - limited the use of hardcoded strings especially for strings used in multiple test functions refactored test to reduce code repetition implemented suggested changes from pull request -removed addSpinStateToLog porpery from within for loop - change constants declared with static int to constexpr - remove unused variables in test methods change some variable names to match mantid naming cinvention
This is a squashed version of mantidproject#37166 Add group peak workspaces model Add show data to group peak workspaces Add group presenter and table model Add support for sorting, deleting, plotting, stats, and batch loading Add group model unit tests Add group table model unit tests Add group presenter unit tests Add release note
This is a squashed version of mantidproject#38173 add file watcher to script repo model If the script repo json files are deleted / moved, the ScriptRepository object will be set as invalid and the user will be prompted to reinstall upon reloading. added release note add repo model test ccpCheck fixes change test to remove dir check for file existence on isValid rather than using watcher [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci cppcheck changes move ui setup to avoid crash when installing for the first time move repo_ptr setup back inside try to fix python test remove now unneeded changes fix cppcheck return by const ref
for more information, see https://pre-commit.ci
This is a squashed version of mantidproject#38564 Clear events in frame if not good frame In preparation for next frame Update cpp check suppression The condition ++numWordsSkipped is always true, but it will only get evaluated if !file.seekg(SKIP_WORD_SIZE, std::ios_base::cur).eof() is true which will indicate a word being skipped, so this does actually have a function! Update test to assert no events if min=max=0 Add release note Fix cpp check supression
This is a squashed version of mantidproject#38459 fix typo to trigger cppcheck run Remove cppcheck suppressions temporarily for cppcheck update Update cppcheck to 2.16.0 Suppress class_X_Y cppcheck warnings. Restrict cppcheck to unix only. There is no 2.16.0 for Windows. We can re-add it for Windows in the future if it is reinstated. For the time being, since nobody is running it locally, we don't really need it to be installed on Windows. Sort cppcheck supressions by file name and line number Group internal cppcheck errors to extract them from the bounty list Update cppcheck suppressions for v2.16.0
for more information, see https://pre-commit.ci
…_16_ornlnext Update cppcheck to 2.16.0 - ornlnext
peterfpeterson
force-pushed
the
37875_boost_optional
branch
from
47acae2 to
6306a59
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
As part of the modernization work laid out in #37875, this migrates more of the reflectometry interface from
boost::optionaltostd::optional.The things being intentionally skipped use the following classes as optional:
stringwhich is involved in theboost::optional<boost::optional<string>>and breaks testsdoublewhich prompts changes in the sip bindingsItemLookupCriteriaErrorLookupRowProcessingInstructionsRowRowLocationSubtreeTo test:
This is a refactor so all tests should pass, but poking around with the ISIS reflectometry GUI is a good idea.
This does not require release notes.
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
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill 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.