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

Relax() stopping criterion (issue #146). #148

Merged
merged 8 commits into from
Jan 19, 2018
Merged

Conversation

jsampaio
Copy link
Contributor

Code for issue #146 . This changes the relax() stopping condition so that it stops when the maximum torque reaches a user definable threshold ('RelaxTorqueThreshold').

updating this branch to latest mumax/3
update my fork to match the mumax git
changing stopping criterion
@godsic
Copy link
Contributor

godsic commented Jan 17, 2018

@jsampaio It appears to me this pull request changes Relax() default behavior. Isn't it?

@jsampaio
Copy link
Contributor Author

@godsic This pull request changes the Relax() current behaviour. As it is, relax() does not stop when the system is relaxed, it stops when the system is steadily evolving, i.e. when the dm/dt is constant. The main change of this patch is changing the stopping condition to a dm/dt approaching zero. I've written more details of the things in the current Relax() that I think should change in issue #146 .

@godsic
Copy link
Contributor

godsic commented Jan 17, 2018

@jsampaio thanks for the clarification. Our merging policy is that any changes, apart from the bug fixes, shouldn't change mumax3 default behavior. So I would suggest you to use either RelaxTorqueThreshold = 0. or RelaxTorqueThreshold = -1. as the default value (whatever value feels more logical to you) that reproduces Relax() current behavior. Then, if desired user can override this by setting RelaxTorqueThreshold to the desired value.

Changed to keep previous relax() behaviour by default. When RelaxTorqueThreshold < 0, relax() will stop when the average torque rises (previous behaviour). The default value for  RelaxTorqueThreshold is now -1.
@godsic
Copy link
Contributor

godsic commented Jan 17, 2018

@jsampaio Thanks. I guess we are approaching the point of merging. I basically have three comments:

  • at the moment RelaxTorqueThreshold = -1. and RelaxTorqueThreshold = 0 are equivalent, both will trigger the default behaviour. Is it intentional?
  • to minimize possible regressions please try to keep the mumax3 original code as it is.
  • have you tested your changes against unit tests?

@JeroenMulkers
Copy link
Collaborator

The Relaxation procedure is an essential part of mumax3, so I think it's best not to be too hasty in modifying anything.

Consider the following statement:

Stop the relaxation if the
(1) average
(2) maximum
of the torque
(a) doesn't change any more
(b) is approximately zero

Basically, we have a '2D binary option space'. In the current version we use option (1a), which should indeed be the default if we add other options in order to ensure BC.

If we add option (2b) as suggested by @jsampaio, I think we should include the other options as well. That being said, I am not yet convinced that adding option (b) is necessary. @jsampaio, do you have a script which clearly demonstrates that (2b) is a better stopping criteria than (1a)?

@jsampaio
Copy link
Contributor Author

@godsic : I still think that the default (current) behaviour of relax() is not desirable, as it frequently yields results very far away from a relaxed state. Perhaps if you guys agree, in the future, we can change the default RelaxTorqueThreshold so to use the new relax() condition, if we're all satisfied that it poses no new problems.

I have a demonstrator simulation, comparing the old and the new relax, and Minimize() : relaxTriangularTrackSimplified3.txt

In the current code, any value of RelaxTorqueThreshold <=0 will use the default behaviour. I think this is ok, as the threshold for the maximum torque should always be a positive finite number.

I've run the tests on the folder /test. However, I get some errors with both branches of the code, on tests that do not have relax()...

@JLeliaert
Copy link
Contributor

I agree with @jsampaio that the current relax function sometimes does not lead to an energy minimum and an improved version is welcome. Even if we want to keep the default behaviour as it is now, I think we should include the new version with the RelaxTorqueThreshold option.

@jsampaio
Copy link
Contributor Author

@JeroenMulkers you've summarised the problem very neatly. My opinion is that option (2b) is better than the others, if we want that Relax() drive the system to a local energy minimum. Let me explain what I think is bad with the other options:

  • The average torque (options 1a 1b) suffers from numerical noise. Near equilibrium, most spins will be static, and so the average will be a sum of a large number of near-zero values. This means that Relax() would stop even if there was still one spin moving quickly, as long as there's also lots of static spins. Moreover, the final state would change with the total number of spins in the system, as a bigger system would have more static spins adding noise to the average.

  • Waiting for the torque to be constant (options 1a and 2a) is not a condition for a local energy minimum. A system that is steadily evolving, but still far away from the energy minimum, can show a constant torque.

There are lots of cases where the current Relax() (with condition 1a) stops too early. I've attached an example in the previous comment: a domain wall in a wedged track.

@JeroenMulkers
Copy link
Collaborator

JeroenMulkers commented Jan 17, 2018

@jsampaio thank you for the script. It will definitely be useful to improve the relax function!

I agree with you that a converging criteria based on the maximum of the torque instead of the average torque might be better in some cases.

I also want to point out that if you have a constant torque, then this constant has to be zero because the magnetization can only make rotations on the unit sphere. This means that it is a good condition to check if the magnetization is in a local energy minimum.

@jsampaio
Copy link
Contributor Author

@JeroenMulkers : That's only true for the torque vector. In the case of relax(), if there's a constant torque, that constant can be different from zero. It's not the torque vector we test, it's the magnitude of the torque vector. Moreover, we consider either the maximum or the average of the torque over all the cells.

Take the case of a moving domain wall at constant speed. The maximum torque (magnitude) will be constant but correspond to different cells as the DW moves. The average torque (magnitude) will also be constant.

@JeroenMulkers
Copy link
Collaborator

I see. It is indeed the magnitude of the torque which is checked and which could be a constant. Thank you for pointing that out!

@godsic
Copy link
Contributor

godsic commented Jan 18, 2018

@jsampaio Would you be so kind to replace lines 85-95 in your most recent relax.go implementation with exact copy of the master's code and I am happy to merge then.

default behaviour is now exactly the previous one.
@godsic godsic merged commit fd1a693 into mumax:master Jan 19, 2018
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.

4 participants