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

2531 add mumag to tools #2825

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

2531 add mumag to tools #2825

wants to merge 55 commits into from

Conversation

Funkmich008
Copy link
Collaborator

@Funkmich008 Funkmich008 commented Mar 15, 2024

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.

  • import data from the test data
  • press the buttons

Review Checklist:

[if using the editor, use [x] in place of [ ] to check a box]

Documentation (check at least one)

  • There is nothing that needs documenting
  • Documentation changes are in this PR
  • There is an issue open for the documentation (link?)

Installers

  • There is a chance this will affect the installers, if so
    • Windows installer (GH artifact) has been tested (installed and worked)
    • MacOSX installer (GH artifact) has been tested (installed and worked)

Licencing (untick if necessary)

  • The introduced changes comply with SasView license (BSD 3-Clause)

@Funkmich008 Funkmich008 linked an issue Mar 15, 2024 that may be closed by this pull request
@lucas-wilkins lucas-wilkins added the Discuss At The Call Issues to be discussed at the fortnightly call label Mar 15, 2024
Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, and MuMagPerpendicularGeo.py, should live in a subdirectory of sas/sascalc Any plotting, visualization, and GUI elements should remain within sas/qtgui.
  • All data sets used by this package should live in sas/example_data, again in their own sub-directory. All data sets in the example_data subpackage 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/MuMagTool and Utilities/MuMagTool/UI directories should have an __init__.py file. 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.

src/sas/qtgui/Utilities/MuMagTool/MuMagParallelGeo.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagLib.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMagTool/MuMagParallelGeo.py Outdated Show resolved Hide resolved
@lucas-wilkins lucas-wilkins marked this pull request as draft March 26, 2024 14:15
Copy link
Member

@butlerpd butlerpd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lucas-wilkins removed the Discuss At The Call Issues to be discussed at the fortnightly call label May 9, 2024
@lucas-wilkins lucas-wilkins marked this pull request as ready for review June 10, 2024 16:30
@lucas-wilkins
Copy link
Contributor

@Funkmich008 Think everything is done. We still need to sort out the docs.

@lucas-wilkins lucas-wilkins added the Discuss At The Call Issues to be discussed at the fortnightly call label Jun 11, 2024
@lucas-wilkins lucas-wilkins dismissed butlerpd’s stale review June 20, 2024 12:55

should be fixed

@lucas-wilkins lucas-wilkins removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 2, 2024
@lucas-wilkins lucas-wilkins removed their request for review July 2, 2024 12:54
@lucas-wilkins
Copy link
Contributor

TODO: set the icon

@rozyczko
Copy link
Member

rozyczko commented Oct 28, 2024

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 :(
Not even magtool own CSV files from Utilities\MuMag\SANSData

@lucas-wilkins
Copy link
Contributor

lucas-wilkins commented Oct 28, 2024 via email

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please either treat MuMagLib as a class, or remove the class entirely. Another option is to have these three methods inside simple_fit()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I just disagree with this one. I think static classes are neater.

src/sas/qtgui/Utilities/MuMag/media/mumag_help.rst Outdated Show resolved Hide resolved
* **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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You misread it - but the fact you misread it is evidence that it was unclear. Have tried to make it clearer.

src/sas/qtgui/Utilities/MuMag/models.py Show resolved Hide resolved
src/sas/qtgui/Utilities/MuMag/MuMag.py Show resolved Hide resolved
Comment on lines +67 to +68
except Exception as e:
raise LoadFailure(f"Failed to load from {directory}: " + repr(e))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add MuMag to tools
5 participants