Minor Bugfixes

[mducle]

A bunch of minor bugfixes which are either suggested by users or originally reported by them.

• Implement fixes suggested for sw_plotspec in Auto mode in sw_plotspec overrides 'imag' option #172
• Converts single q-point hkl to column to fix Unclear documentation about calculating spinwave spectra (or potentially a python regression) #179
• Remove inversion of user-inputed colormap in sw_plotspec to fix Inverted colormaps in sw_plotspec #131
• Refactor sw_plotspec to avoid recursive calls - split off main plotting stuff into an internal function which in order to fix User specified arguments in sw_plotspec ignored if 'mode', 'auto' (the default) used #132
• Implement fixes suggested for spinw.fitspec in Bugs in fitspec #98
• Add spinw object to the spectrum object in fitspec to fix fitspec: cannot handle twins #158

[github-actions]

Test Results

    4 files  ± 0    108 suites  +2   8m 0s ⏱️ - 7m 23s
  686 tests  - 17    668 ✅  - 17  18 💤 ±0  0 ❌ ±0 
1 916 runs   - 30  1 880 ✅  - 30  36 💤 ±0  0 ❌ ±0 

Results for commit 3ce1cbe. ± Comparison against base commit 74422b7.

This pull request removes 19 and adds 2 tests. Note that renamed tests count towards both.

sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_general_hkl
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_hamiltonian
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_hamiltonian_squared
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_hamiltonian_white
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_egrid)
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_instrument)
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_neutron)
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_omegasum)
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_plotspec)
sw_tests.system_tests.systemtest_spinwave_symbolic_nips ‑ test_symbolic_spectra(test_spectra_function_name=sw_tofres)
…

sw_tests.unit_tests.unittest_spinw_fitspec ‑ test_fitspec
sw_tests.unit_tests.unittest_spinw_fitspec ‑ test_fitspec_twin

♻️ This comment has been updated with latest results.

[RichardWaiteSTFC]

sw_plotspec errors on main branch when spinwave calculated using fastmode=true as doesn't have a copy the spinw object (I think because it turns on fitmode )

Unrecognized field name "obj".

Error in sw_plotspec (line 225)
unitL = spectra.obj.unit.label{1};

I haven't had a chance to tests this yet, but from a quick glance at the diff it looks like this may still be present - if so would it would make sense to fix it in this PR right?

[mducle]

@RichardWaiteSTFC Yes, I think it makes sense to have it in this PR. I'll try to fix the sw_plotspec bug next week or if you can make a fix before then, just push it to this branch.

[RichardWaiteSTFC]

I have been able to reproduce #174 on master using this code adapted from tutorial 8

AF33kagome = spinw;
AF33kagome.genlattice('lat_const',[6 6 40],'angled',[90 90 120],'spgr','P -3');
AF33kagome.addatom('r',[1/2 0 0],'S', 1,'label','MCu1','color','r');
AF33kagome.gencoupling('maxDistance',7);
AF33kagome.addmatrix('label','J1','value',1.00,'color','g');
AF33kagome.addcoupling('mat','J1','bond',1);
S0 = [0 0 -1; 1 1 -1; 0 0 0];
AF33kagome.genmagstr('mode','helical','k',[-1/3 -1/3 0],...
    'n',[0 0 1],'unit','lu','S',S0,'nExt',[3 3 1]);

kag33Spec = AF33kagome.spinwave({[-1/2 0 0] [0 0 0] [1/2 1/2 0] 250},'hermit',false);

sw_plotspec(kag33Spec,'mode','auto','axLim',[0 2.5],'colorbar',false',...
    'colormap',[0 0 0],'imag',false,'sortMode',true,'dashed',true)

This is indeed fixed in this branch, but if you set imag=true it throws this error

Unrecognized function or variable 'modeList'.

Error in sw_plotspec>plotspec_internal (line 511)
                    hLegend(modeList+1) = hPlot(end);

Error in sw_plotspec (line 268)
        [fHandle0, pHandle0] = plotspec_internal(spectra, param);

which isn't thrown on main

[RichardWaiteSTFC]

Running the code in #179

FMchain = spinw;
FMchain.genlattice('lat_const',[3 8 8],'angled',[90 90 90])
FMchain.addatom('r', [0 0 0],'S', 1,'label','MCu1','color','blue')
FMchain.gencoupling('maxDistance',7)
FMchain.addmatrix('value',-eye(3),'label','Ja','color','green')
FMchain.addcoupling('mat','Ja','bond',1);
FMchain.genmagstr('mode','direct', 'k',[0 0 0],'n',[1 0 0],'S',[0; 1; 0]);
Q = sym([0 1 0]);
FMchain.spinwave(Q)

now throws a different error on this branch

Error using cell2mat
CELL2MAT does not support cell arrays containing cell arrays or objects.

Error in spinw/spinwave (line 480)
hkl    = cell2mat(hkl);

it still works for no symbolic

[mducle]

Thanks @RichardWaiteSTFC - I'll try to fix these issues and do better testing! Will push something next week...

[RichardWaiteSTFC]

Thanks @RichardWaiteSTFC - I'll try to fix these issues and do better testing! Will push something next week...

No worries, I can also help out next week - I'll keep testing and post more comments if I find anything...

[RichardWaiteSTFC]

Running tutorial 8 code again (see #180 (comment)) with this plotting command

sw_plotspec(kag33Spec,'mode','auto','axLim',[0 2.5],'colorbar',false',...
    'colormap',[0 0 0],'sortMode',true,'dashed',true)

now throws this error

Error using sw_plotspec>plotspec_internal
The dimensions of the colormap should be [3 nPlot]

Error in sw_plotspec (line 241)
        [fHandle0, pHandle0] = plotspec_internal(spectra, param);

However if you convert the colormap to a column vector - i.e. [0,0,0]' - it works as expected and changes the colormap to greyscale (which was previously ignored on main branch).

Basically I think this is because you fixed #132 - but it does indicate that at one point it was possible to pass a row vector here?

[RichardWaiteSTFC]

I was looking into testing #131 - it looks to me like jet is inverted (normally red is high and blue is low?)
Also this may not be due to your change but I think we should probably set 0 to white. I know white can also appear in a colormap (e.g. blue-white-red) but I think in this use-case such colormaps are not very suitable

sw_plotspec(kag33Spec,'mode','auto','axLim',[0 2.5],'colorbar',false',...
    'colormap',@jet,'sortMode',true,'dashed',true) % [0 0 0]

[RichardWaiteSTFC]

Thanks for this Duc, the sw_plotspec code looks much simpler to follow! And thanks for adding a test for sw_fitspec! Just a few small things above. I can also look into these next week I know you're busy!

Also 472e474 needs to removed.

[mducle]

@RichardWaiteSTFC Thanks for the review - I think I fixed most issues raised...

Also 472e474 needs to removed.

I think if we squash and merge, git is smart enough not to merge this twice, so it should be ok.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 11.36364% with 78 lines in your changes are missing coverage. Please review.

Project coverage is 40.73%. Comparing base (735d30a) to head (3ce1cbe).
Report is 10 commits behind head on master.

Files	Patch %	Lines
swfiles/sw_plotspec.m	0.00%	58 Missing ⚠️
swfiles/sw_mex.m	0.00%	14 Missing ⚠️
swfiles/@spinw/spinwave.m	66.66%	3 Missing ⚠️
swfiles/@spinw/fitspec.m	33.33%	2 Missing ⚠️
swfiles/sw_egrid.m	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   40.51%   40.73%   +0.22%     
==========================================
  Files         240      239       -1     
  Lines       15981    15810     -171     
==========================================
- Hits         6474     6440      -34     
+ Misses       9507     9370     -137     

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

[RichardWaiteSTFC]

Looks good thanks - thanks also for updating the changelog!