Test sw_egrid

[rebeccafair]

No description provided.

[github-actions]

Test Results

       4 files     102 suites   18m 52s ⏱️
   612 tests    612 ✔️ 0 💤 0 ❌
1 738 runs  1 738 ✔️ 0 💤 0 ❌

Results for commit e1186b5.

♻️ This comment has been updated with latest results.

[rebeccafair]

As input I've used the 1D chain [qh 0 0] spectrum from the spinwave tests, though it might be nice to have a 2 magnetic atom system too, especially for testing the modeIdx argument. There seems to be a bug when component is Px or a fieldname (fName), unless I'm using them incorrectly! For now I've documented this behaviour in the test_Px and test_fName_component tests, checking that an error is raised.

There is a bit of inconsistent behaviour with the output structure where extra fields Mab, Pab, intP and some parameters are initialised when using the Sperp component, whereas they aren't initialised when a different component is used. This probably isn't a big deal, but this is why there are separate sw_egrid_out_sperp and sw_egrid_out test structs.

[RichardWaiteSTFC]

As input I've used the 1D chain [qh 0 0] spectrum from the spinwave tests, though it might be nice to have a 2 magnetic atom system too, especially for testing the modeIdx argument. There seems to be a bug when component is Px or a fieldname (fName), unless I'm using them incorrectly! For now I've documented this behaviour in the test_Px and test_fName_component tests, checking that an error is raised.

Thanks for documenting these, I think I'll try and address them in this PR

There is a bit of inconsistent behaviour with the output structure where extra fields Mab, Pab, intP and some parameters are initialised when using the Sperp component, whereas they aren't initialised when a different component is used. This probably isn't a big deal, but this is why there are separate sw_egrid_out_sperp and sw_egrid_out test structs.

I wonder if that's because those fields ( Mab, Pab, intP) are in the output spectrum struct after running sw_neutron?

[RichardWaiteSTFC]

Looks great, it's almost here thanks!
As a note to myself here are the things still to do:

• Test input parameters: epsilon, autoEmin and imagChk
• Test modeIdx with an example with two magnetic atoms
• passing a cell array of compoinents e.g. 'component',{'Sxx+Syy' 'Szz'}

Is there anything else you think I've missed?

[codecov-commenter]

Codecov Report

Merging #130 (40fbc1d) into master (80d30ff) will increase coverage by 0.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
+ Coverage   38.67%   39.22%   +0.55%     
==========================================
  Files         239      239              
  Lines       15829    15823       -6     
==========================================
+ Hits         6122     6207      +85     
+ Misses       9707     9616      -91     

Impacted Files	Coverage Δ
swfiles/sw_egrid.m	83.82% <100.00%> (+20.75%)	⬆️

... and 2 files with indirect coverage changes

[RichardWaiteSTFC]

In the failing test
system_tests.systemtest_spinwave_incommensurate_and_supercell_consistency/test_AFM_kagome
There are still some zero-energy modes that have the odd eigenvalue that is ~10^-3 (i.e. not quite zero) - which is getting to the limit where we don't want to neglect DSF from all eigenvalues below this threshold because valid modes may have such eigenvalues (e.g. near Gamma).

I propose to filter out these modes 9or provide the user the option to do so) when calculating swConv but it's a case of finding a code set of criteria. I can think of the following:

1. Threshold fraction of eigenvalues <1e-6 (i.e. much smaller than typical resolution in data) ?
2. Such modes tend to have DSF with extremely large values ~10^7 (relative to e.g. median?)

Whatever approach we take we want to ensure we can discriminate between valid modes with v. small eigenvalues and won't mis-behave if few q-bins.

[RichardWaiteSTFC]

I think I should highlight this, it produces slightly different behaviour for autoEmin=True,

spinw/swfiles/sw_egrid.m

Lines 468 to 476 in 40fbc1d

	if param.autoEmin
	if ~exist('ioMax','var')
	ioMax = max(abs(imag(omega{1}(:))));
	end
	if abs(ebin_edges(1)) < ioMax
	ebin_edges(1) = ebin_edges(1)+ioMax;
	ebin_cens(1) = ebin_cens(1)+ioMax/2;
	end
	end

but I think it is an improvement on what was there before

spinw/swfiles/sw_egrid.m

Lines 465 to 472 in df2a40d

	if param.autoEmin && abs(Evect(1)-dE(1)/2)<epsilon
	if ~exist('ioMax','var')
	ioMax = max(abs(imag(omega{1}(:))));
	end
	Evect(1) = Evect(1)+ioMax+2*epsilon;
	eEvect(1)= eEvect(1)+ioMax+2*epsilon;
	dE(1) = dE(1)-(ioMax+2*epsilon);
	end

which mis-used epsilon (now deprecated in this PR but was a tolerance for regular binning) and made ebin centers and edges inconsistent.

However, I'm not completely happy as it doesn't fix all the problems - there are still the following issues (that exist with the old code as well):

1. If imgChk=False then there is no guarantee that the ioMax < bin width so the first bin edge could be moved beyond the second
2. The energy bin edges are sorted, but this fails to account for the fact the first energy bin could be negative and span the elastic line (probably unlikely though)

[mducle]

I think we're almost there. The changes look good. I've just got a request to fix the Px bug. Regarding your comments about the zero energy boundary

I propose to filter out these modes 9or provide the user the option to do so) when calculating swConv but it's a case of finding a code set of criteria. I can think of the following:

1. Threshold fraction of eigenvalues <1e-6 (i.e. much smaller than typical resolution in data) ?
2. Such modes tend to have DSF with extremely large values ~10^7 (relative to e.g. median?)

Whatever approach we take we want to ensure we can discriminate between valid modes with v. small eigenvalues and won't mis-behave if few q-bins.

I think option 2 is better - so maybe a check for eigenvalues below some threshold and with some intensity above some threshold?

However, I'm not completely happy as it doesn't fix all the problems - there are still the following issues (that exist with the old code as well):

1. If imgChk=False then there is no guarantee that the ioMax < bin width so the first bin edge could be moved beyond the second
2. The energy bin edges are sorted, but this fails to account for the fact the first energy bin could be negative and span the elastic line (probably unlikely though)

Maybe if imgChk=false we don't move the bin edges at all?

Not sure what to do about the second issue... let's see if it becomes a big problem later...

[RichardWaiteSTFC]

I think option 2 is better - so maybe a check for eigenvalues below some threshold and with some intensity above some threshold?

Have added a parameter maxDSF to do this.

Maybe if imgChk=false we don't move the bin edges at all?

Done, I agree I think this is good enough for now

[mducle]

Looks good! Thanks!