Add unit tests for gencoupling #89

RichardWaiteSTFC · 2022-06-28T14:29:47Z

Added unit tests for gencoupling
Added validation for positive distances
Added validation for maxDistance > dMin
Fixed bug by checking if any bonds have length < maxSym
Renamed tol -> tolMaxDist (breaking change to API, but param was undocumented anyway - checked with @mducle )

Have found an issue with gencoupling when symmetry of lattice is lower than spacegroup (e.g. orthohombic lattice and tetragonal spacegroup) - in this instance the sapcegroup is essentially ignored and the symmetry equivalence is determined by bond length only. This can be fixed by calling swsym.bond with tol = inf (otherwise symmetry generated bonds are not returned if bond length not within tolDist)
https://github.com/SpinW/spinw/pull/89/files#diff-5e8b49a4d3f0f8b0cb0615a63d7e12571ec1d1e1babfe11bb75c18fee8833ed1R248
However I think this problem is best handled in validation in genlattice (shouldn't be able to specify tetragonal spacegroup with orthorhombic lattice parameters - it's not physical!)

Partially Fixes #72

codecov-commenter · 2022-06-28T14:33:06Z

Codecov Report

Merging #89 (70511d2) into master (aeb4fdc) will increase coverage by 0.13%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   24.45%   24.59%   +0.13%     
==========================================
  Files         235      235              
  Lines       15641    15663      +22     
==========================================
+ Hits         3825     3852      +27     
+ Misses      11816    11811       -5

Impacted Files	Coverage Δ
swfiles/@spinw/gencoupling.m	`100.00% <100.00%> (+4.80%)`	⬆️

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 aeb4fdc...70511d2. Read the comment docs.

github-actions · 2022-06-28T14:38:44Z

Unit Test Results

    4 files ±  0   56 suites +4 3m 37s ⏱️ +29s
156 tests +21 156 ✔️ +21 0 💤 ±0 0 ❌ ±0
620 runs +84 620 ✔️ +84 0 💤 ±0 0 ❌ ±0

Results for commit 70511d2. ± Comparison against base commit aeb4fdc.

♻️ This comment has been updated with latest results.

Not sure if this is functioning as expected. Bonds along a and b are sym. equiv. accroding to P 4 group but the lengths differ by d=0.5 Ang. Here an error is thrown when d < tol but surely that is then consistent (i.e. tol is tolerance in bond lengths considered equiv.). Also I think it should actually be tolDist (tolD variable in code) not tol used in this comparison.

Check that when symetry of lattice is higher than spacegorup provided the bonds of same length are correctly indexed as inequivilent based on spacegroup symmetry operations.

rebeccafair

Looks good! Just the odd comment, and I would add 1 simple test with 2 magnetic atoms to make sure nothing weird goes on with the matrix sizes, and add the validation for maxDistance > dMin and bonds length < maxSym to the changelog.

swfiles/@spinw/gencoupling.m

RichardWaiteSTFC · 2022-07-01T09:34:28Z

+sw_tests/+unit_tests/unittest_spinw_gencoupling.m

+            expected_coupling.atom1 = int32([1 2 2 1 1 2 2]);
+            expected_coupling.atom2 = int32([2 1 1 1 1 2 2]);
+            expected_coupling.mat_idx = zeros(3, 7, 'int32');
+            expected_coupling.idx = int32([1 2 2 3 3 3 3]);


@mducle in this test I have 2 magnetic ions and the bonds between atom_1-atom_1 and atom_2-atom_2 in adjacent cells have the same bond index (3) - see table below for summary. As they are different species I would not have thought these bonds were symmetrically equivalent (despite having the same length) . Is this a bug?

idx subidx dl dr length matom1 idx1 matom2 idx2 ___ ______ ___________ _______________________ ______ __________ ____ __________ ____ 1 1 0 0 0 0.25 0.25 0.25 1.639 {'atom_1'} 1 {'atom_2'} 2 2 1 1 0 0 0.75 -0.25 -0.25 2.681 {'atom_2'} 2 {'atom_1'} 1 2 2 0 1 0 -0.25 0.75 -0.25 2.681 {'atom_2'} 2 {'atom_1'} 1 3 1 1 0 0 1 0 0 3 {'atom_1'} 1 {'atom_1'} 1 3 2 0 1 0 0 1 0 3 {'atom_1'} 1 {'atom_1'} 1 3 3 1 0 0 1 0 0 3 {'atom_2'} 2 {'atom_2'} 2 3 4 0 1 0 0 1 0 3 {'atom_2'} 2 {'atom_2'} 2

it still happens even if you call atom_2->Ce3+ (for example)

I think this is not a bug, because spgr is not specified for this case - the default setup in line 18 just specifies the lattice constant, and in this case the default behaviour is to sort bonds by length only.

If I change the setup to:

swobj = spinw(); % default init swobj.genlattice('lat_const',[3 3 5],'spgr','P 4') swobj.addatom('r',[0 0 0],'S',1) swobj.addatom('r',[0.5 0.5 0.5],'S',2) swobj.gencoupling('maxDistance', 5); swobj.table('bond', 1:3)

I get the two atom_1-atom_1 and atom_2-atom_2 bonds being different (note I had to move atom_2 to [0.5, 0.5, 0.5] as otherwise the symmetry would add an extra 3 atom_2s around the unit cell...

Now, I guess we could have a discussion as to whether this behaviour (treat all bonds with the same length the same irregardless of what atoms they connect) is the correct one... e.g. even if no symmetry is specified when we know that two atoms are different then should we make a distinction? For backwards compatibility reason I'm inclined to leave the behaviour as it is... but I can see the point to change it too...

Correct there's no spacegroup provided in this example, but I expected bonds between the same pair of species to be labelled as symmetrically equivalent if they have the same bond-length. I would not have expected the atom_1-atom_1 bond to be equivalent to atom_2-atom_2 as they are different species. But maybe I've misunderstood?

Yeah, I see your point - even though we don't have any spacegroup information we still know that the atoms are different so the bonds ought to be inequivalent. I guess the behaviour here is the result of the history of the code - I think the distance sorting was implemented first and then they symmetry checking was done on top of it...

The question now is whether to change it - my concern is about backwards compatibility - I'll have a look at the documentation and tutorials to see if any examples rely on this behaviour...

So tutorial 31 is the only published tutorial which relies on this behaviour - and it's just through sheer laziness in not specifying the atom labels (they should be the same type of atom). There are some other tutorials which are not published which rely on this too...

Now, we could do a couple of things:

Keep the current behaviour

Keep the current behaviour but emit a warning; maybe tell users to explicitly label their atoms if they haven't done so.

Change the behaviour to distinguish different atom labels except the case of default labels (e.g. atom_<x>).

Change the behaviour to distinguish different atom labels in all cases and emit a warning stating the default behaviour has changed

Change the behaviour to distinguish different atom labels in all cases and stay silent.

What do you think? Can you think of any other options?

We could do 2. for now and say that the behaviour will change in the future, create an issue and do 4. or 5. in a future release? I guess it depends if you consider this a bug, or a 'feature'...

It might be nice to have a 3.1.3 or 3.2 release of SpinW without any major changes except the slight improvements in warnings/consistency etc. that we're finding through the unit tests. We could discuss this in the Monday catch-up?

As discussed offline, this bug will effect users (even if they have specified atom labels) so it is worth fixing. We've decided to go for option 4. The downside is this is a breaking change, although we can warn users about this in the CHANGELOG and in the code. It might break some systems tests etc. so depending on the size of the change and how much it breaks we may end up doing this in a separate PR.

Ah sorry @rebeccafair didn't see your previous comment!

OK after talking to @rebeccafair lets discuss on Monday!

mducle

Looks good to me. 👍

Maybe the inequivalent-atom-at-same-distance issue could be put into a separate issue/PR?

RichardWaiteSTFC · 2022-07-01T10:55:10Z

Looks good to me. 👍

Maybe the inequivalent-atom-at-same-distance issue could be put into a separate issue/PR?

Have raised a separate issue #90

RichardWaiteSTFC added 10 commits May 23, 2022 14:08

Add validation to ensure max < min distance and valid bond lengths

61401cf

Add doc string for undocumented parameter of gencoupling

e1fd197

Add unit tests for gencoupling

d248b45

Merge branch 'master' into 72_add_unit_tests_for_gen_spinw_methods

e2e26b6

Change to use default coupling struct and verify_val

f1c8a0b

Add tests for forcenoSym, fid and atom dist < dMin

0845f73

Validate that all distances must be positive and add test

2293787

Fix bug if no bond length > maxSym (now throw warning) and add test

eee7385

Add more detailed test for maxSym option when multiple bonds

9c07fed

Fix typo in warniong string

45a25d1

RichardWaiteSTFC marked this pull request as draft June 28, 2022 14:29

RichardWaiteSTFC force-pushed the 72_add_unit_tests_for_gen_spinw_methods branch from 817a273 to 593cd47 Compare June 28, 2022 15:39

RichardWaiteSTFC added 2 commits June 29, 2022 16:11

Use tolDist for equivalence if spacegrp and change tol to tolMaxDist

497a8a3

Update test with P2 sym to check inequiv. bonds with same length

13eca3c

Check that when symetry of lattice is higher than spacegorup provided the bonds of same length are correctly indexed as inequivilent based on spacegroup symmetry operations.

RichardWaiteSTFC force-pushed the 72_add_unit_tests_for_gen_spinw_methods branch from 304d281 to 13eca3c Compare June 29, 2022 16:41

RichardWaiteSTFC marked this pull request as ready for review June 30, 2022 07:50

RichardWaiteSTFC added 2 commits June 30, 2022 09:14

Throw error if tolerances larger than dMax (and added tests)

627286e

Update changelog with breaking change to API (param name changed)

576b52c

RichardWaiteSTFC requested review from rebeccafair and mducle June 30, 2022 08:29

rebeccafair requested changes Jun 30, 2022

View reviewed changes

swfiles/@spinw/gencoupling.m Outdated Show resolved Hide resolved

RichardWaiteSTFC added 3 commits July 1, 2022 10:25

Add test for two atoms (mag/non-mag) in unit cell

f750ffd

Update changelog with added validation

52d729e

Add line breaks to long error msg in gencoupling

70511d2

RichardWaiteSTFC commented Jul 1, 2022

View reviewed changes

mducle approved these changes Jul 1, 2022

View reviewed changes

RichardWaiteSTFC mentioned this pull request Jul 1, 2022

Gencoupling creates sym. equivalent bonds between different pairs of species #90

Open

rebeccafair approved these changes Jul 1, 2022

View reviewed changes

mducle merged commit 674dd30 into master Jul 1, 2022

mducle mentioned this pull request Jul 1, 2022

Increase SpinW CI coverage #72

Open

38 tasks

RichardWaiteSTFC deleted the 72_add_unit_tests_for_gen_spinw_methods branch August 17, 2022 13:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for gencoupling #89

Add unit tests for gencoupling #89

RichardWaiteSTFC commented Jun 28, 2022 •

edited

Loading

codecov-commenter commented Jun 28, 2022 •

edited

Loading

github-actions bot commented Jun 28, 2022 •

edited

Loading

rebeccafair left a comment

RichardWaiteSTFC Jul 1, 2022 •

edited

Loading

mducle Jul 1, 2022 •

edited

Loading

RichardWaiteSTFC Jul 1, 2022 •

edited

Loading

mducle Jul 1, 2022

mducle Jul 1, 2022

rebeccafair Jul 1, 2022

RichardWaiteSTFC Jul 1, 2022

RichardWaiteSTFC Jul 1, 2022

RichardWaiteSTFC Jul 1, 2022

mducle left a comment •

edited

Loading

RichardWaiteSTFC commented Jul 1, 2022

Add unit tests for gencoupling #89

Add unit tests for gencoupling #89

Conversation

RichardWaiteSTFC commented Jun 28, 2022 • edited Loading

codecov-commenter commented Jun 28, 2022 • edited Loading

Codecov Report

github-actions bot commented Jun 28, 2022 • edited Loading

Unit Test Results

rebeccafair left a comment

Choose a reason for hiding this comment

RichardWaiteSTFC Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

mducle Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

RichardWaiteSTFC Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

mducle Jul 1, 2022

Choose a reason for hiding this comment

mducle Jul 1, 2022

Choose a reason for hiding this comment

rebeccafair Jul 1, 2022

Choose a reason for hiding this comment

RichardWaiteSTFC Jul 1, 2022

Choose a reason for hiding this comment

RichardWaiteSTFC Jul 1, 2022

Choose a reason for hiding this comment

RichardWaiteSTFC Jul 1, 2022

Choose a reason for hiding this comment

mducle left a comment • edited Loading

Choose a reason for hiding this comment

RichardWaiteSTFC commented Jul 1, 2022

RichardWaiteSTFC commented Jun 28, 2022 •

edited

Loading

codecov-commenter commented Jun 28, 2022 •

edited

Loading

github-actions bot commented Jun 28, 2022 •

edited

Loading

RichardWaiteSTFC Jul 1, 2022 •

edited

Loading

mducle Jul 1, 2022 •

edited

Loading

RichardWaiteSTFC Jul 1, 2022 •

edited

Loading

mducle left a comment •

edited

Loading