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

Changing user experience #105

Merged
merged 14 commits into from
Apr 10, 2024
Merged

Changing user experience #105

merged 14 commits into from
Apr 10, 2024

Conversation

GuiMacielPereira
Copy link
Collaborator

@GuiMacielPereira GuiMacielPereira commented Mar 13, 2024

Description of work:
After meeting with Matthew, it was clear that a different way of interacting with the code was needed, as described in #104. The main changes this PR introduces are the following:

  • Creation of new experiments is now done through manual copy-and-paste of a single analysis_inputs.py script
  • Terminal functionality is still available but options have changed from cache, experiment and ipfolder to just inputs and ipfolder. This makes it harder to set up experiments through the terminal but makes it simpler to set up experiments when running an analysis_inputs.py script.

To test:

  1. Create a new conda enviroment: conda create -n mvesuvio-test
  2. Activate environment: conda activate mvesuvio-test
  3. Install mantidworkbench (version needs to be 6.8): mamba install mantidworkbench==6.8
  4. cd into the repository with this PR and install package by running pip install -e .
  5. In the terminal, run the command mvesuvio config to set up default configuration
  6. Go to the default folder, ~/.mvesuvio and copy analysis_inputs.py file
  7. Create a new directory anywhere in your machine and paste analysis_inputs.py file inside it.
  8. Open Mantid workbench from the vesuvio conda environment (command workbench)
  9. Inside workbench, open the analysis_inputs.py file you just pasted.
  10. Run the file.
  11. The file should run without issues and create a new folder with the same name as the file, i.e. check that a folder called analysis_inputs is created alongside the file analysis_inputs.py.
  12. Check the folder contains the outputs of the inputs file.
  13. Copy-and-paste the same analysis_inputs.py file in the same directory but this time rename it to something different
  14. Run this file from inside workbench and check that a new directory with the new name is created.

Fixes #104.

- Replaced cache and experiments by only one argument: inputs
- CLI working as expected
- Introduced modifyied __name__ == "__main__" statement
- Also accounts for when the script is run from within the mantid editor
- Previously the path to the experiment directory was defined in terms of scriptName
- scriptName was defined in two places, which is not ideal
- Defined scriptName in a single place, ICHelpers.py
Mantid editor caches variables in the module scope, leading to wrong behaviours

The changes implemented in this commit fix this by calling the variables inside functions
- Changed the way system tests are run by using new functunality
- Now tests use inputs from the same file by setting the inputs in mvesuvio config
- Bootstrap and Jacknife tests are still ignored, to be corrected in the future
- The previous approach of writing the version to __init__.py overwrites the file
- Safer to write version to unique file named _version.py
- Don't really know how the versioning works on github so I am still figuring that out
- New version of mantid caused libtiff5 to go missing again
- Pinned mantid to 6.8
- Updated Ubuntu just because
- Deleted file that was being used to set paths for testing
@GuiMacielPereira GuiMacielPereira linked an issue Mar 13, 2024 that may be closed by this pull request
@MialLewis
Copy link
Collaborator

We really need to get Matthew using conda, as it’s our primary supported installation method!

The risk with installing the package this way is that there’s no guarantee that pip won’t break the mantid install, or that the mvesuvio package will work correctly.

@SilkeSchomann
Copy link
Collaborator

When I run analysis_inputs.py there is a pop-up window in the end asking whether to clean up all the workspaces. Entering n gives the following error message: ```
KeyboardInterrupt: Run of procedure canceled.
File "C:/Users/eoc57742/testmvesuvio/copy_analysis_inputs.py", line 173, in
mvesuvio.run()
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio_init_.py", line 53, in run
main(run_args)
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio\scripts_init_.py", line 16, in main
_run_analysis(args.yes)
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio\scripts_init
.py", line 92, in __run_analysis
analysis_runner.run(yes_to_all)
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio\analysis_runner.py", line 25, in run
runScript(
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio\vesuvio_analysis\run_script.py", line 98, in runScript
checkUserClearWS(yes_to_all) # Check if user is OK with cleaning all workspaces
File "C:\Users\eoc57742\vesuvio2024\vesuvio.\mvesuvio\vesuvio_analysis\run_script.py", line 118, in checkUserClearWS
raise KeyboardInterrupt("Run of procedure canceled.")


Is this behaviour intended?

@SilkeSchomann
Copy link
Collaborator

The folders are created as expected, by the way, both with the original script and also after renaming.

@GuiMacielPereira
Copy link
Collaborator Author

Ah, yes, this is intended, although I should probably change this in the future. The first time you run the script, the user should go with 'y' and delete the workspaces that were just loaded into mantid. This will reload the same workspaces but from a local path. In general I put this warning so that the user does not overwrite workspaces accidentally but this is an edge case where this warning should probably not be displayed.

@SilkeSchomann
Copy link
Collaborator

I've repeated step 6. to 14. with a folder on one drive and it also worked as expected.

@GuiMacielPereira GuiMacielPereira merged commit 7faf09a into main Apr 10, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New user facing option required
3 participants