Add new option "Hsteps" to HysteresisDriver

[newton-per-sqm]

Allows configuration of tunable (non-symmetric) hysteresis loops. (According to our discussion in PR #137)

Short Example:

import oommfc as oc
# define the system [...]
hd= oc.HysteresisDriver()
hd.drive(system, Hsteps=[
    # [Hstart, Hend, n]
    [(0,0,0), (0,0,1), 10],
    [(0,0,1), (0,0,0.5), 20],
])
# or (compatible to previous versions)
hd.drive(system, Hmin=(0,0,1), Hmax=(0,0,-1), n=20)

I've adjusted the kwargs check to not allow the definition of both options ( [Hmin, Hmax, n] AND Hsteps). But for compatibility, the old concept of defining symmetric hysteresis loops still works as expected.

[lang-m]

Apologies for the late reply. Thank you for adding the requested changes. I have left a few minor comments.

I have two further requests:

1. The additional docstring in your initial proposal looked good and is currently missing. Please re-add the part that you deleted in this commit: 5775c75.
2. We will need an additional test for the new functionality. Testing is done with the separate micromagnetictests package and the test should be added to micromagnetictests/hysteresisdriver.py.

[newton-per-sqm]

Sorry for the late reply, had some other work to do. I've included all your suggested changes, seems all valid to me 👍. But I'm not quite sure how to handle your suggestion in #138 (comment). As far as I understood, there is already WIP in implementing this new way of accepting kwargs directly as an dictionary and not anymore via an unpacking operator?

I've also included the docstring from the old version.

[newton-per-sqm]

Mostly accepted, but not sure how to handle https://github.com/ubermag/oommfc/pull/138/files/142d1c163aaa565b0e33ce644e8018284f6ad448#r1323102185 without breaking functionality.

[newton-per-sqm]

Please try a pipeline running your currently defined unittests. All current tests regarding HysteresisDriver should pass as in previous versions, function signature and therefore functionality is unchanged.

[newton-per-sqm]

I'm now (after unittest failed) surprised why it worked in my case, as it is now clear to me why it cannot. 😂
https://github.com/ubermag/oommfc/actions/runs/6643512793/job/18070713189?pr=138#step:5:216

For running with (Hmin, Hmax, n) as keywords, _checkargs(...) is now constructing a new keyword Hsteps for a symmetric hysteresis simulation out of (Hmin, Hmax, n), as intended by @lang-m. kwargs is getting updated, but this update is unused, til now not returned.

IMHO we have two options:

• Revert the idea of a single code path in the scripts module --> scripts has to check wether (Hmin, Hmax, n) or Hsteps as keywords are given and produce a mif file for both scenarios. _checkargs is checking beforehand which keywords are given and will raise an error accordingly, such that the scripts module has not to worry about it.
• The updated kwargs dictionary has to be returned from the _checkargs function to be used afterwards. This has to be done in https://github.com/newton-per-sqm/oommfc/blob/c089c7847c16bcd6df8d6f5a097ac40f1631c69e/oommfc/drivers/driver.py#L59 by returning a drive_kwargs dictionary and using it afterwards. I think this is a bad idea, as it is necessary to change all drivers for this, and also has to be implemented with higher effort --> drive_kwargs has to be passed through many methods?

[lang-m]

Thanks for clarifying (I was really surprised that it might have worked for you before).

I'll think about the possible alternatives and come back to you (or propose an implementation directly). Not sure when I'll have time for this. I'll try to reply soon (maybe within the next few weeks or so).

I would propose that you start adding a test in here: https://github.com/ubermag/micromagnetictests/blob/master/micromagnetictests/calculatortests/hysteresisdriver.py when you have time because fully integrating that will also take some time.

[lang-m]

Hi @newton-per-sqm apologies for the late reply.
I have now modified the behaviour of _checkargs so that we can use it to update kwargs (5d5c3ac). The changes are merged into the master branch. Could you please merge (or rebase) master into your branch and then update the kwargs handling as discussed before.

[lang-m]

I will merge the new test in ubermag/micromagnetictests#46 once this PR is ready.

[codecov]

Codecov Report

Attention: Patch coverage is 45.45455% with 12 lines in your changes missing coverage. Please review.

Project coverage is 92.70%. Comparing base (dfbf32e) to head (142d1c1).
Report is 1 commits behind head on master.

❗ Current head 142d1c1 differs from pull request most recent head ef493d7

Please upload reports for the commit ef493d7 to get more accurate results.

Files	Patch %	Lines
oommfc/drivers/hysteresisdriver.py	40.00%	9 Missing ⚠️
oommfc/scripts/driver.py	57.14%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
- Coverage   93.28%   92.70%   -0.59%     
==========================================
  Files          26       26              
  Lines        1192     1192              
==========================================
- Hits         1112     1105       -7     
- Misses         80       87       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[newton-per-sqm]

Hi @newton-per-sqm apologies for the late reply. I have now modified the behaviour of _checkargs so that we can use it to update kwargs (5d5c3ac). The changes are merged into the master branch. Could you please merge (or rebase) master into your branch and then update the kwargs handling as discussed before.

Hi @lang-m! Thanks for your work here. Unfortunately, it looks like the update mechanism for the kwargs dictionary is currently not working as expected. Just a short summary for myself what we are targeting here:

• _checkargs(self, kwargs) has to construct a new key Hsteps out of Hmin, Hmax and n if and only if these three parameters are defined in kwargs. The dictionary is getting updated and given back to the main routine.
• If Hsteps is already given, we only need to check all of its "steps" for completeness.
• The goal is, that the file scripts/driver.py only needs to use Hsteps to construct the hysteresis simulation out of it and casts it into the mif file, no more using of Hmin, Hmax and n in addition here.
• But exactly this last file / method is currently not seeing any update in kwargs, because the _checkargs() method is called elsewhere and the changed kwargs variable is not getting passed through the whole call stack.

We could overcome this issue quickly by calling _checkargs() within scripts/driver.py again. But that seems to be more of a quick and dirty solution. We should investigate the path of the changed kwargs variable ...

-> As commented below, I think this happens because the drive_kwargs_setup() method of the Driver class in drivers/driver.py is was not returning the updated kwargs dictionary. Also the drive() method in micromagneticmodel
/driver.py is not yet changing kwargs and needs also to overwrite the variable.

[lang-m]

@samjrholt I would like to merge this before we merge #153

[lang-m]

@newton-per-sqm We would like to include this new feature in the next release of Ubermag, which we plan for the second half of June. The implementation and testing are done (I will take care of any remaining test failures should there be any).

For that we would need/want a few more things:

1. An updated/extended example in the form of a Jupyter notebook. The best place I think would be https://github.com/ubermag/tutorials/blob/master/examples/hysteresis.ipynb. Could you please add an additional section to that notebook and show/explain the new feature.
2. An entry for the changelog in ubermag/ubermag.github.io with a 1-2 sentence summary and a reference to this PR. Please create a PR based on the current changelog branch with the changelog branch as target.
3. Add you to the list of contributors in the README (if you want). For that please add your name and affiliation to this file: https://github.com/ubermag/devtools/blob/master/repometadata/authors.toml. I will then take care of the rest.