ubermag / micromagneticmodel

Python-based domain-specific language for computational magnetism.
http://ubermag.github.io
BSD 3-Clause "New" or "Revised" License
10 stars 8 forks source link

`__eq__` based on same class and name for `mm.Term` abstract class #46

Closed swapneelap closed 2 years ago

swapneelap commented 2 years ago

This helps in differentiating between two terms of the same class but with different names. This is particularly useful for the mm.Container.__sub__ method.

I encountered a problem when removing an additional Zeeman field with name=pulse from system.energy like system.energy -= pulse where pulse = mm.Zeeman(H=H_pulse, name="Pulse"). The subtraction would remove the first term of mm.Zeeman class which was a constant external field and not the pulse.

swapneelap commented 2 years ago

Sounds good to me. I will use _allowed_attributes to check equality.

lang-m commented 2 years ago

We only use the term name to distinguish between different terms in the energy/dynamics equation. I think it would therefore make sense to only compare the names. (I agree from a programming point of view it would make more sense to compare all attributes.)

I think we should also modify the Container class. There are several lines like term.name == other.name. I think these should become term == other. Here, we can only compare the names because we need unique names for the mif file, so in my opinion Term.__eq__ should only use the term name.

lang-m commented 2 years ago

@swapneelap We should add an additional test for this situation.

marijanbeg commented 2 years ago

I would prefer Pythonic comparison for __eq__. If we wanted only to compare names, we could then just write directly in the code self.name == other.name. We use it mostly to remove terms from containers, so removing the term would be a bit more explicit, e.g.:

my_zeeman = mm.Zeeman(...)
system.energy += my_zeeman
# ....
system.energy -= my_zeeman
codecov-commenter commented 2 years ago

Codecov Report

Merging #46 (e1033b9) into master (ee3d7da) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #46   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines          452       441   -11     
=========================================
- Hits           452       441   -11     
Impacted Files Coverage Δ
micromagneticmodel/abstract/container.py 100.00% <100.00%> (ø)
micromagneticmodel/abstract/term.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ee3d7da...e1033b9. Read the comment docs.

swapneelap commented 2 years ago

I have updated the term.__eq__() method to take into account the term._allowed_attributes. As discussed in the meeting, this might create a little confusion for the users while adding the tems to a container. Now, even though two terms are not "equal", they still cannot be added to the container if they belong to the same class and have the same name. I have also updated the exception message for the container.__add__() to help the user if such a situation arise. I think in the long run, comparing terms like this will be benificial. I have also added tests corresponding to the changes.

marijanbeg commented 2 years ago

Looks good to me.