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

Merge FreeMagnetismInterface feature enhancement #232

Open
wants to merge 66 commits into
base: master
Choose a base branch
from

Conversation

acaruana2009
Copy link
Contributor

FreeMagnetismInterface acts in the same way that FreeInterface does. It allows for a freeform connecting spline between magnetic layers to deal with non-trivial magnetic interfaces.

Additional modifications have been put into mono.py in order to make it more useable, however, more work is probably needed in all of the spline functions we have in refl1d - this may be out of scope of this PR.

Paul Kienzle and others added 30 commits November 17, 2020 17:29
Also fixed issue with magnetic profile calculation when drhoM[-1] = 0.
…rface

FIX: length of vectors not equal when dthetaM is not defined
Removed conditions at lines 514 and 521. Also added same statement at line 520 as is used for rhoM. This should now behave in the same way as FreeInterface.
Removed divide by zero when z[-1]=0.
…ue when not specified

If only tabove or tbelow are specified then they are made equal. If neither are specified they are made the same as DEFAULT_THETA_M.

This is to get around the issue when interfacing with a non-magnetic layer where the thetaM value should not change.
Additional edits as suggested by Paul K.
Also fixed issue with magnetic profile calculation when drhoM[-1] = 0.
Removed conditions at lines 514 and 521. Also added same statement at line 520 as is used for rhoM. This should now behave in the same way as FreeInterface.
Removed divide by zero when z[-1]=0.
…ue when not specified

If only tabove or tbelow are specified then they are made equal. If neither are specified they are made the same as DEFAULT_THETA_M.

This is to get around the issue when interfacing with a non-magnetic layer where the thetaM value should not change.
Additional edits as suggested by Paul K.
Also fixed issue with magnetic profile calculation when drhoM[-1] = 0.
Removed conditions at lines 514 and 521. Also added same statement at line 520 as is used for rhoM. This should now behave in the same way as FreeInterface.
Removed divide by zero when z[-1]=0.
…ue when not specified

If only tabove or tbelow are specified then they are made equal. If neither are specified they are made the same as DEFAULT_THETA_M.

This is to get around the issue when interfacing with a non-magnetic layer where the thetaM value should not change.
Additional edits as suggested by Paul K.
Removed conditions at lines 514 and 521. Also added same statement at line 520 as is used for rhoM. This should now behave in the same way as FreeInterface.
…ue when not specified

If only tabove or tbelow are specified then they are made equal. If neither are specified they are made the same as DEFAULT_THETA_M.

This is to get around the issue when interfacing with a non-magnetic layer where the thetaM value should not change.
Additional edits as suggested by Paul K.
…gnetism-interface-AJC

# Conflicts:
#	refl1d/mono.py
When updating the branch to the latest version from the master branch a lot of merge conflicts had to be gone through. Here I am correcting where I accepted minor changes that do not matter to the code but deviate from the master branch unnecessarily.
@acaruana2009 acaruana2009 self-assigned this Nov 21, 2024
"""

name: Optional[str]
mbelow: Optional[Union[Parameter, float]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmaranville I am not sure if this is the current style that you are using for typing. I.e. would you prefer to remove Any and Optional?

Copy link
Member

@bmaranville bmaranville Nov 21, 2024

Choose a reason for hiding this comment

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

It's not really a question of style - the dataclass attribute types should be exactly what will be stored in those attributes after init + post_init. There can be different types for the arguments of the init function, if they are going to be coerced before being stored in the attribute. For example, an argument will often be of type Union[float, Parameter] but then we store it as Parameter.default(value) which coerces it to a Parameter, so Parameter should be the type of the attribute (not the Union)

Copy link
Contributor Author

@acaruana2009 acaruana2009 Nov 21, 2024

Choose a reason for hiding this comment

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

OK so in this case, we could drop the Optional since we probably don't want None as one of the types? That said tabove and tbelow do take None as the default arg.

Copy link
Member

Choose a reason for hiding this comment

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

The typing of the arg is not the same as the typing of the attribute - tbelow and tabove are both initialized as Parameter objects during init, even if they are None as an argument. The type of the attributes should be Parameter

@bmaranville
Copy link
Member

@acaruana2009 have you tried the new class out? Does it work as you expect? Can you serialize/deserialize it?

@acaruana2009
Copy link
Contributor Author

@acaruana2009 have you tried the new class out? Does it work as you expect? Can you serialize/deserialize it?

Yes, it all seems to work. I haven't done any fits with it yet, but I can load in a file, save out into a session and reload into a session and it keeps chisq intact.

@bmaranville bmaranville self-assigned this Nov 22, 2024
…meter after init, even ones that are supplied as float to the init function)
refl1d/mono.py Outdated
dp: List[Union[float, Par]]
dz: List[Par]
dp: List[Par]
magnetism: Optional[BaseMagnetism] = None
Copy link
Member

Choose a reason for hiding this comment

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

Can this really be any BaseMagnetism object, or does it have to be specifically a FreeMagnetismInterface?

@bmaranville bmaranville force-pushed the magnetism-interface-AJC-webview branch from 4f50fbc to 689653a Compare December 16, 2024 17:23
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.

3 participants