Minor Bugfixes - Githubissues

mducle commented 4 months ago

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

Implement fixes suggested for sw_plotspec in #172
Converts single q-point hkl to column to fix #179
Remove inversion of user-inputed colormap in sw_plotspec to fix #131
Refactor sw_plotspec to avoid recursive calls - split off main plotting stuff into an internal function which in order to fix #132
Implement fixes suggested for spinw.fitspec in #98
Add spinw object to the spectrum object in fitspec to fix #158

github-actions[bot] commented 4 months ago

Test Results

4 files ± 0 108 suites +2 8m 0s :stopwatch: - 7m 23s 686 tests - 17 668 :white_check_mark: - 17 18 :zzz: ±0 0 :x: ±0 1 916 runs - 30 1 880 :white_check_mark: - 30 36 :zzz: ±0 0 :x: ±0

Results for commit 3ce1cbec. ± Comparison against base commit 74422b73.

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 ```

:recycle: This comment has been updated with latest results.

RichardWaiteSTFC commented 4 months ago

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 commented 4 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

RichardWaiteSTFC commented 3 months ago

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 commented 3 months ago

Running tutorial 8 code again (see https://github.com/SpinW/spinw/pull/180#issuecomment-2117764803) 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 commented 3 months ago

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]

mducle commented 3 months ago

@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 commented 3 months ago

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 :warning:
swfiles/sw_mex.m	0.00%	14 Missing :warning:
swfiles/@spinw/spinwave.m	66.66%	3 Missing :warning:
swfiles/@spinw/fitspec.m	33.33%	2 Missing :warning:
swfiles/sw_egrid.m	66.66%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SpinW / spinw

Minor Bugfixes #180

Test Results

Codecov Report