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

Insert marketplacemodel bulk ferromagnets #592

Merged
merged 25 commits into from
Sep 26, 2024

Conversation

astellhorn
Copy link
Contributor

I have added the marketplace model "SANS in bulk ferromagnets". It consists of the files "magnetic_functions.c", "micromagnetic_FF_3D.c", and "micromagnetic_FF_3D.py". In the issue "Insert model "SANS of bulk ferromagnets" from the model marketplace #2784", I have mentioned that it would be good to (i) add the parameter descriptions for the choosable parameters in the GUI (e.g. radius = "Radius of the core", thickness = "Thickness of shell"), and (ii) to add the description of the whole model (given in the .py file) to the sasview webpage. Unfortunately, for both I do not know how to do this - could you have a look on that?

have inserted the option for chains (taken form MagneticOrientedCHains) into the parallelepiped script from sasview
…magnetic_FF_3D.py from the marketplace. Unsure how to insert the parameter descriptions given in micromagnetic_FF_3D.py to the GUI according to sasview issue #2784
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.

I have not fully reviewed the docs or functionality yet. However some of the failing tests can be traced to the problems noted here.

Another question, perhaps for everyone, should we move magnetic_functions.c to /lib and make them part of the available library routines? That would probably require some documentation updates (regarding available library functions)?

sasmodels/models/micromagnetic_FF_3D.py Show resolved Hide resolved
sasmodels/models/micromagnetic_FF_3D.py Outdated Show resolved Hide resolved
@dehoni
Copy link
Contributor

dehoni commented May 21, 2024

MacOs Failing not finding Python 3.7. It wasn't me!

@butlerpd
Copy link
Member

we recently dropped support for testing on python 3.9 because mac runners didn't support it. We apparently did not update sasmodel runners. 3.7, 3.8, AND 3.9 should be removed. We should add 3.11. I'll open an issue for this.

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.

It now loads and runs and produces reasonable curves as far as I can tell not being a magnetic person. I note the name attribute however is different from the python and c file names. The simple load seems to work but my recollection is that we still have places which use the file name instead of the name attribute? which is why they are always the same.

Is there are reason they are not the same?

Also, looking at the functions in magnetic_functions.c I really think these should be moved to the library as they are definitely more general than this model. Moreover it means that if people want to make a change and distribute on the marketplace, they won't need to carry the magnetic_functions.c around.

Note: the mac errors should go away once we merge the latest changes to the release branch

@butlerpd butlerpd changed the base branch from master to release_1.0.8 July 15, 2024 23:31
@butlerpd
Copy link
Member

@butlerpd to talk to @pkienzle about how to handle

Copy link
Collaborator

@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 can't speak to the correctness of the model, but I do have a few comments in other parts. The limitation set by sasmodels, which only allows magnetic analysis on 2D data, is limiting you, which, I assume, is why you are recreating many of the built-in magnetic functions. Your approach might be a way to bridge the gap to allow more magnetic analysis in the future.

sasmodels/models/lib/magnetic_functions.c Outdated Show resolved Hide resolved
sasmodels/models/lib/magnetic_functions.c Outdated Show resolved Hide resolved
sasmodels/models/micromagnetic_FF_3D.py Outdated Show resolved Hide resolved
sasmodels/models/micromagnetic_FF_3D.py Show resolved Hide resolved
sasmodels/models/micromagnetic_FF_3D.py Outdated Show resolved Hide resolved
@pkienzle
Copy link
Contributor

A more general question for something like a magnetic cylinder model which has orientation in both the nuclear and the magnetic scattering.

For non-magnetic models, kernel_iq.c rotates the model into the canonical orientation and calls Iqac for shapes with rotational symmetry or Iqabc for more general shapes. This removes the need for every model to implement the orientation transformation.

The underlying model here is a sphere so no need to reorient the shape. Instead it is using the Iqxy without any nuclear orientation parameters, but two magnetic orientation parameters.

If we try to implement magnetism on an oriented shape, then either we will need to move the rotation from (qx, qy, qz=0) back into Iqxy function, or we will need some way to indicate that the magnetic orientation parameters need to be rotated in the same way as the nuclear parameters so we can determine the projection into the scattering plane.

A related issue is dispersion in magnetic orientation. The main reason we converted to the Iqabc form was to handle orientational dispersion consistently regardless of the orientation of the shape. The solution was to apply two separate rotations, first applying the dispersion rotation, then rotating according to orientation. Actually, the inverse since we are rotating the measured (qx,qy,qz) into (qa, qb, qc) for the shape in canonical orientation.

With magnetic models implemented using Iqxy we will need to include dispersion parameters in the model parameter table and include the dispersion loop directly in the code.

@dehoni
Copy link
Contributor

dehoni commented Sep 14, 2024

Here is a Mathematica notebook from the original paper that can produce testdata.

testdata_Michels_Honecker_2013.zip

@krzywon
Copy link
Collaborator

krzywon commented Sep 25, 2024

Based on the feedback from our call yesterday, the only outstanding issue here is the failing Ubuntu build. The unit tests for the model pass. I am working on the Ubuntu build in release_1.0.8_ubuntu_amd, so I think this can be merged. @pkienzle, can you give one final look, please?

@krzywon krzywon merged commit dfe40d8 into release_1.0.8 Sep 26, 2024
12 checks passed
@krzywon krzywon deleted the insert-marketplacemodel-bulk-ferromagnets branch September 26, 2024 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants