Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor Bugfixes #180

Merged
merged 10 commits into from May 20, 2024
Merged

Minor Bugfixes #180

merged 10 commits into from May 20, 2024

Conversation

mducle
Copy link
Member

@mducle mducle commented Apr 25, 2024

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

Copy link

github-actions bot commented Apr 25, 2024

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.

@mducle mducle force-pushed the bugfixes_apr2024 branch 9 times, most recently from 97c49c5 to 9a6e079 Compare April 26, 2024 14:09
Update base github-actions to node-20 (v4)
   gha/[up|down]load-artifact no longer accept
   same named artifacts for a matrix run
Update Matlab CI to v2
   lowest supported version is R2021a
Update sw_mex to support Apple-Silicon (arm64)
Fix bug in INSTALL_DEPS in yaml
Inc pcsmo test tolerance to pass on MacOS-14
@RichardWaiteSTFC
Copy link
Collaborator

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
Copy link
Member Author

mducle commented May 2, 2024

@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.

spinwave() now creates a .obj field regardless
  When fitmode=true, this a barebones struct
  When fitmode=false, this is a full spinw object
Fix issue with sw_plotspec when using fastmode/fitmode
Add basic test for fitspec() to check twins handling
@RichardWaiteSTFC
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

mducle commented May 17, 2024

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

@RichardWaiteSTFC
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

RichardWaiteSTFC commented May 17, 2024

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]

image

Copy link
Collaborator

@RichardWaiteSTFC RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

mducle commented May 19, 2024

@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
Copy link

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.

Copy link
Collaborator

@RichardWaiteSTFC RichardWaiteSTFC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks - thanks also for updating the changelog!

@mducle mducle merged commit 3e8b492 into master May 20, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants