ubermag / oommfc

OOMMF calculator.
http://ubermag.github.io
BSD 3-Clause "New" or "Revised" License
50 stars 17 forks source link

Mif-file generation broken #108

Closed lang-m closed 2 years ago

lang-m commented 2 years ago

The changes in micromagneticmodel.term.__contains__ (checking for all attributes) introduced in ubermag/micromagneticmodel#46 breaks the mif file generation. In driver.py:driver_script we use mm.Damping() in system.dynamics tests (similar for mm.Precession). Without the correct arguments these tests fail and we write wrong alpha values to the mif file.

marijanbeg commented 2 years ago

Although physically probably it does not make sense to have two damping terms, I would not exclude that possibility in mm. The reason is to allow full flexibility for any calculators that can be built on top of mm. I believe damping terms just like energy terms have names already. Instead of mm.Damping() in system.dynamics, we could have if any(isinstance(term, mm.Damping) for term in system.dynamics):.... Does that make sense?

marijanbeg commented 2 years ago

Similarly, when we check if a term has a unique name, we could ask for:

if term.name not in [i.name for i in system.energy]:
    # add the term to the container
lang-m commented 2 years ago

I think it would be nice to encapsulate that functionality to avoid all the hard-to-understand if statements. @swapneelap and I had a discussion and decided to add two additional methods:

>>> system.dynamics = mm.Damping(alpha=1)
>>> system.dynamics.contains_by_type(mm.Damping)
True
>>> system.dynamics.get_by_type(mm.Damping)
[mm.Damping(alpha=1)]

Instead of contains_by_type we could generalise __contains__ to accept a type:

>>> system.dynamics = mm.Damping(alpha=1)
>>> mm.Damping() in system.dynamics  # wrong alpha
False
>>> mm.Damping in system.dynamics  # check for type
True

but we think that this is to "dangerous" because the difference is very subtle.

@marijanbeg Does that sound okay?

lang-m commented 2 years ago

Update to my previous comment:

>>> system.dynamics.contains(type=mm.Damping)
True
>>> system.dynamics.get(type=mm.Damping)
[...]
marijanbeg commented 2 years ago

Yes, I like this idea, and also agree that extending in is too dangerous.

  1. I do not like the names, e.g. type is a built-in and contains is too similar to __contains__ special method.
  2. Maybe type can be cls.
  3. Maybe contains can be contains_cls
  4. What if there are multiple terms of the same class, e.g. Zeeman? Do you get a Container as the result? Is the result always the container or it can be a term object?
  5. I am wondering if get is redundant. For instance, if not system.dynamics.get(cls=mm.Damping) can be trythy/falsy.

There are no right/wrong answers here - just some topics to think about. Whatever you decide, I would be happy with :)

lang-m commented 2 years ago
  1. I agree type shadows the built-in type inside the method but we don't use build-in type here and it is exactly what we are asking for, the type of term we are looking for.
  2. I picked contains intentionally because of the similarity.
  3. The get method always returns a simple list. We often need the first (and only) element and that is easy with lists (which can be empty if there is no term of the correct type).
  4. You mean contains is not necessary?

I'd be okay to go with just get that returns a Python list and can be used inside if.

>>> if system.dynamics.get(type=mm.Damping):
>>>     system.dynamics.get(type=mm.Damping)
[mm.Damping(...)]
marijanbeg commented 2 years ago
  1. +1
  2. +1
  3. +1
  4. Yes, I meant contains is not necessary :)