SpinW / spinw

SpinW Matlab library for spin wave calculation
http://www.spinw.org
GNU General Public License v3.0
35 stars 15 forks source link

Add a function to save a slice in MDH format #149

Closed granrothge closed 1 year ago

granrothge commented 1 year ago

This function will take the output of a spectra and create a slice in an MDH format. Note the "Instrument" name is hard coded as a valid instrument is needed for Mantid to read the file. I'd prefer this to be "SpinW", but since that is not a valid Mantid instrument it can't be read by Mantid. I would like to see this requirement relaxed in Mantid, but that is a different discussion.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 21.89% and project coverage change: +0.22% :tada:

Comparison is base (415d143) 38.67% compared to head (32cb8b7) 38.90%. Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #149 +/- ## ========================================== + Coverage 38.67% 38.90% +0.22% ========================================== Files 239 240 +1 Lines 15829 15971 +142 ========================================== + Hits 6122 6213 +91 - Misses 9707 9758 +51 ``` | [Files Changed](https://app.codecov.io/gh/SpinW/spinw/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW) | Coverage Δ | | |---|---|---| | [swfiles/private/sw\_issymspec.m](https://app.codecov.io/gh/SpinW/spinw/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW#diff-c3dmaWxlcy9wcml2YXRlL3N3X2lzc3ltc3BlYy5t) | `54.54% <0.00%> (-20.46%)` | :arrow_down: | | [swfiles/sw\_instrument.m](https://app.codecov.io/gh/SpinW/spinw/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW#diff-c3dmaWxlcy9zd19pbnN0cnVtZW50Lm0=) | `2.36% <0.00%> (-0.16%)` | :arrow_down: | | [swfiles/sw\_spec2MDHisto.m](https://app.codecov.io/gh/SpinW/spinw/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW#diff-c3dmaWxlcy9zd19zcGVjMk1ESGlzdG8ubQ==) | `0.00% <0.00%> (ø)` | | | [swfiles/sw\_egrid.m](https://app.codecov.io/gh/SpinW/spinw/pull/149?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW#diff-c3dmaWxlcy9zd19lZ3JpZC5t) | `85.71% <100.00%> (+22.64%)` | :arrow_up: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/SpinW/spinw/pull/149/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=SpinW)

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

granrothge commented 1 year ago

@mducle All Good points. I will update accordingly.

granrothge commented 1 year ago

@mducle I think I have changed things to match the requested changed. Let me know if anything else is needed.

RichardWaiteSTFC commented 1 year ago

Thanks for this @granrothge! I just had a quick test and i think I still see the same issues as @mducle .

  1. With the slice going along two different symmetry directions

    sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 0 0] [0 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
    sw_spec2MDHisto(sp, [1 0 0; 0 1 0; 0 0 1], [0.01, 0.01, 0.01], 'testmdh.nxs');

    it produces this workspace in mantid image i.e. it has 200 bins all along K. I actually think with the limitations of MDHisto workspaces in mantid that we can't do better than that...do you have a requirement to support such slices? If not maybe we only support slices along a single Q direction? Or if you do need to support these slices we could print a warning to the user so they know what to expect in the .nxs file?

  2. The following code

    sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 1] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
    sw_spec2MDHisto(sp, [1 0 0; 0 1 0; 0 0 1], [0.01, 0.01, 0.01], 'testmdh3.nxs');

    produces this error

    
    Error using reshape
    Number of elements must not change. Use [] as one of the size
    inputs to automatically calculate the appropriate size for that
    dimension.

Error in sw_spec2MDHisto (line 81) signal = reshape(dat,Dszs);

RichardWaiteSTFC commented 1 year ago

I was wondering does the proj matrix need to have the normalised unit vectors? Or maybe I'm mis-interpreting it...? When I specify a slice along [110] from the doc string I gather 110 needs to be in the proj matrix, but when I try

sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1 1 0; -1 1 0; 0, 0, 1], [0.01, 0.01, 0.01], 'testmdh3.nxs');

I get the same error as above. If I normalise the vectors in proj like so

sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1/sqrt(2) 1/sqrt(2) 0; -1/sqrt(2) 1/sqrt(2) 0; 0, 0, 1], [0.01, 0.01, 0.01], 'testmdh3.nxs');

I don't get the error, but the algorithm seems to hang and the .nxs file is not closed (so can't be opened in mantid). Am I doing something wrong? If so maybe some more info in the doc string and a more informative error would be helpful!

granrothge commented 1 year ago

@RichardWaiteSTFC @mducle OK Let me do some more rigorous testing. As to question 1. I Do not envision doing more than one direction at a time. I will at least add some documentation and may add an error catch.

granrothge commented 1 year ago

@mducle @RichardWaiteSTFC I think I have addressed the off axis case. Good catch. I also added a prototype of a test script that provides which directions should be tested. I'm not familiar with the testing setup so it will likely need to be modified to actually run.

RichardWaiteSTFC commented 1 year ago

Thanks for the changes - I think there may still be a bug with determining the x-axis of the slice from the proj matrix provided. If I make this cut along [110]

swo = sw_model('triAF', 1);
sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1 1 0; -1 1 0; 0, 0, 1], [0.01, 0.01, 0.01], 'testmdh.nxs');

And plot it in mantid I get a cut along [1-10] image

I think there may also be an issue with the normalisation of the basis vectors - or at least if you supply a normalised proj matrix when the argument for sw_egrid isn't normalised like so

sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1/sqrt(2) 1/sqrt(2) 0; -1/sqrt(2) 1/sqrt(2) 0; 0, 0, 1], [0.01, 0.01, 0.01], 'testmdh3.nxs');
sw_plotspec(sp)

you get this image But of course the Bragg peak isn't at (0.7,0.7.0) it's at (1,1,0)...

RichardWaiteSTFC commented 1 year ago

I think there may also be an issue with the normalisation of the basis vectors - or at least if you supply a normalised proj matrix when the argument for sw_egrid isn't normalised like so

sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1/sqrt(2) 1/sqrt(2) 0; -1/sqrt(2) 1/sqrt(2) 0; 0, 0, 1], [0.01, 0.01, 0.01], 'testmdh3.nxs');
sw_plotspec(sp)

you get this

image

But of course the Bragg peak isn't at (0.7,0.7.0) it's at (1,1,0)...

Ah having looked at the doc string I think I needed to update the dproj argument - I tried this

sp = sw_egrid(sw_neutron(spinwave(swo, {[0 0 0] [1 1 0] 100})),'component','Sperp', 'Evect', linspace(0,5,200));
sw_spec2MDHisto(sp, [1/sqrt(2) 1/sqrt(2) 0; -1/sqrt(2) 1/sqrt(2) 0; 0, 0, 1], [0.01*sqrt(2), 0.01, 0.01], 'testmdh3.nxs');

but I got the same result - I would expect the end x value to be sqrt(2)

granrothge commented 1 year ago

I thought I had fixed the issue with the negative axis. I will look into it as well as the normalization issues.

granrothge commented 1 year ago

Aah here is the problem with the projection. It must be in columns not rows.

proj = [[1 1 0]' [1 -1 0]' [0 0 1]']
ans =

     1     1     0
     1    -1     0
     0     0     1

not

[1 1 0; -1 1 0; 0, 0, 1]

ans =

     1     1     0
    -1     1     0
     0     0     1
RichardWaiteSTFC commented 1 year ago

Aah here is the problem with the projection. It must be in columns not rows.

Sorry, it does say that in the doc string but I missed it! Thanks for clearing that up!

granrothge commented 1 year ago

@RichardWaiteSTFC @mducle Ok So I think this is now as ready as I can get it.

  1. The test suite now runs and I expanded a few more cases. The files should be read back in and compared. With more understanding of the Matlab test facilities. the test code could be greatly reduced.

  2. I now check that all of the projection vectors are orthogonal to each other (There is a test for this).

  3. I only use the projection vector along the propagation direction to check which vector is the propagation direction. It is over written in the script so the one in the file comes from the spectra object rather than the projection matrix. Similarly the step size input for the propagation direction is over written by what is in the spectra object. I left these in for completeness. This accounts for the normalization issue of the x axis.

  4. the CI is failing because it does not know where to write the files.

granrothge commented 1 year ago

@RichardWaiteSTFC I think it is ready to approve and go.