2531 add mumag to tools #2825
2531 add mumag to tools #2825
Conversation
…hange of button names
…parallel scattering geometry
…attering geometry
…no global variables anymore
…o that the plot got not its own new figure
Funkmich008
requested review from
lucas-wilkins,
krzywon,
astellhorn and
dehoni
lucas-wilkins
added
the
Discuss At The Call
There was a problem hiding this comment.
Hello, @Funkmich008. Thank you for your submission. We appreciate the time you put into this and think the tool will be a great addition to SasView.
As far as acceptance goes, there are a few organizational items I think we will need taken care of before this can be approved:
- We try to keep a clean separation between the calculation elements and gui elements (though we aren't perfect in that matter). Ideally, all calculation elements, which includes the majority of
MuMagLib.py,MuMagParallelGeo.py, andMuMagPerpendicularGeo.py, should live in a subdirectory ofsas/sascalcAny plotting, visualization, and GUI elements should remain withinsas/qtgui. - All data sets used by this package should live in
sas/example_data, again in their own sub-directory. All data sets in theexample_datasubpackage are included in the SasView distributables. I'm not certain the files you included here will be bundled with the release, and if they are, will not be in an obvious location. - The
Utilities/MuMagToolandUtilities/MuMagTool/UIdirectories should have an__init__.pyfile. Our build process expects directories to have this file, otherwise the directory is not bundled in the distributable.
Other suggestions that would greatly help, but could be handled at a later date:
- Documentation: Documentation should include in-code documentation for developrs, and a help file, ideally in the ReST format. You already have a PDF for your MatLab based tool. Updating it for SasView would be a great first step in this.
- Many of your calculation functions use the same subset of arguments. Create a class or series of classes to hold those values and convert the functions to class methods. This could minimize the number of arguments you need to pass.
- Unit tests: Any tests that prove the calculations and GUI intercation behave as expected would be useful. Adding a comment in each calculation function with an expected result for a known set of arguments would be a good first step in this process.
- Any of my inline comments.
There was a problem hiding this comment.
Unfortunately, trying to run the installer on windows fails with the following "unhandled exception in script"
Traceback (most recent call last):
File "sasview.py", line 15, in <module>
File "sas\cli.py", line 161, in main
File "sas\qtgui\MainWindow\MainWindow.py", line 106, in run_sasview
File "sas\qtgui\MainWindow\MainWindow.py", line 42, in __init__
File "PyInstaller\loader\pyimod02_importers.py", line 385, in exec_module
File "sas\qtgui\MainWindow\GuiManager.py", line 38, in <module>
ModuleNotFoundError: No module named 'sas.qtgui.Utilities.MuMagTool'This will clearly need to be fixed. I did not test on macOS but suspect a similar error?
lucas-wilkins
removed
the
Discuss At The Call
|
@Funkmich008 Think everything is done. We still need to sort out the docs. |
lucas-wilkins
added
the
Discuss At The Call
lucas-wilkins
removed
the
Discuss At The Call
|
TODO: set the icon |
|
|
|
There should be a whole bunch of example files. Currently you need to load
a folder, there's three example datasets in qtgui/Utilities/MuMag/SANSData
- final location TDB.
…On Mon, Oct 28, 2024 at 10:33 AM Piotr Rozyczko ***@***.***> wrote:
Import data doesn't show which extensions can be used for import. I can't
actually find a file in our examples, which I can load :(
—
Reply to this email directly, view it on GitHub
<#2825 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACKU4SWWBBMQ6YTVRS3EEBTZ5YAE5AVCNFSM6AAAAABEYCYSQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINBRGE4TSOJXGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
There was a problem hiding this comment.
I've tested and reviewed the code. Overall, things look good from a functionality and code standpoint, but I do have a number of suggestions. The documentation, file loading, and MuMagLib class suggestions are probably necessary. The others would be good to have.
| if datum.applied_field >= parameters.min_applied_field] | ||
|
|
||
| # Brute force check for something near the minimum | ||
| sweep_data = MuMagLib.sweep_exchange_A(parameters, filtered_inputs) |
There was a problem hiding this comment.
Please either treat MuMagLib as a class, or remove the class entirely. Another option is to have these three methods inside simple_fit()
There was a problem hiding this comment.
I think I just disagree with this one. I think static classes are neater.
| * **Analysis method** - This chooses one of two experiment types. Perpendicular is where the applied field is perpendicular to the beam (e.g. beam in x direction and field in z), and parallel where the applied field is parallel. | ||
| * **Maximum q** - MuMag has the ability to exclude q values beyond a given value, specified here | ||
| * **Applied field** - MuMag will use only data with applied field strengths above this value. MuMag requires the sample to be at (or close to) saturation, use this field to specify where this is. | ||
| * **Scan range** - When calculating the exchange stiffness constant A, MuMag's minimisation step has two components. (1) A brute for search, then (2) a refinement. These three connected values that describe the values for which the brute force search will take place, as well as the values that will appear on any plots. |
There was a problem hiding this comment.
typo - (1) A brute for search. Also, I don't understand the last sentence. What three connected values? Only two are described here. Do you mean the other three values? Should the last sentence be separate from the scan range?
There was a problem hiding this comment.
You misread it - but the fact you misread it is evidence that it was unclear. Have tried to make it clearer.
| except Exception as e: | ||
| raise LoadFailure(f"Failed to load from {directory}: " + repr(e)) |
There was a problem hiding this comment.
Any error thrown from any file, regardless the number of successful data loads, will throw away all loaded data. If the try/catch was around the data loading loop, the filename and error could be stored until the data loading is complete and then log the issue.
| ------------ | ||
|
|
||
| To load data click on the `Import Data` button. This will give you a file chooser that allows you to select a | ||
| **directory**. This directory should contain multiple files for measurements take with different applied magnetic fields. |
There was a problem hiding this comment.
Do you mean 'taken' instead of 'take'?
| To load data click on the `Import Data` button. This will give you a file chooser that allows you to select a | ||
| **directory**. This directory should contain multiple files for measurements take with different applied magnetic fields. | ||
|
|
||
| Currently, *the magnetic field and other information is expected to be in filename with a format as described below* |
There was a problem hiding this comment.
Should this be 'in the filename'?
| The CSV files are expected have three columns: momentum transfer :math:`q` in nm :math:`^{-1}`, | ||
| scattering intensity :math:`I(q)`, and the standard error corresponding to `I(q)`. | ||
|
|
||
| Each of the files must have the same length and got to be sorted from the lowest to the highest q-value. |
There was a problem hiding this comment.
Instead of
got to be sorted from...
Should this be:
they need to be sorted from...
Description
The following issue is fixed:
Issue: Add MuMag to tools #2531
Reference DOI to the Paper:
https://doi.org/10.1107/S1600576722005349
Reference Link to the MATLAB interface:
https://files.uni.lu/mumag/
Fixes # (issue/issues)
How Has This Been Tested?
Test run in the developer environment with experimental and synthetic test data.
Review Checklist:
[if using the editor, use
[x]in place of[ ]to check a box]Documentation (check at least one)
Installers
Licencing (untick if necessary)