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

Oriented sesans transform #536

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Oriented sesans transform #536

wants to merge 7 commits into from

Conversation

pkienzle
Copy link
Contributor

@pkienzle pkienzle commented Nov 4, 2022

Allow modelling of SESANS for oriented samples.

This is an old set of changes that was never submitted for review.

I'm opening this PR as a draft so that SESANS developers can discuss it.

Note: there is a corresponding branch in the sasview repository. https://github.com/SasView/sasview/tree/costrafo411

@pkienzle pkienzle marked this pull request as draft November 4, 2022 22:37
@smk78
Copy link
Contributor

smk78 commented Nov 7, 2022

I will bring this to the attention of our SESANS team.

I note that the allied SasView branch is >4000 commits behand main...

@rmdalgliesh
Copy link

I suspect that this is the code that should have been worked on in the last code camp. The sesanscosine.py code contains a lot of modifications that make a lot more sense for TOF-SESANS. They answer quite a lot of the questions I had regarding how the sesans.py code was working for TOF.
I do have some concerns. The call to HankelAccept contains a reference to magfield in order to define the spin-echo length. This doesn't make sense in TOF. If this is required then I would suggest the calculation for SEL should be
SEL= SELC*wavelength^2 where SELC is a constant defined by the parameters of the beamline.

My other concern is the need for a reference for the cosine transform.
The hankel transform is well established and references but I'm not aware of anything where the cosine transform for anisotropic scattering is specifically discussed.

So... my comment is that I think this work and the work from the code camp need to be merged to remove the bugs that were ironed out but it is probably best if those patches are made and then we merge in these changes.

Rob

@lucas-wilkins
Copy link
Contributor

Note, I've opened the corresponding PR in sasview as a draft.

@caitwolf caitwolf self-requested a review November 7, 2022 14:06
Copy link
Contributor

@lucas-wilkins lucas-wilkins left a comment

Choose a reason for hiding this comment

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

Definitely a few things here I'm unsure about, or that could be done better.


repq = np.tile(q, (SElength.size, 1)).T
repSE = np.tile(SElength, (q.size, 1))
cosmat = np.float32(dq / (2 * pi)) * np.cos(repSE * repq)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure an out of the box discrete cosine transform will have time complexity of $N \log N$ rather than the $N^2$ complexity of the transform here.


P = exp(thickness*wavelength**2/(4*pi**2)*(G-G[0]))

def hankel(SElength, wavelength, thickness, q, Iq):
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, the Hankel transform can be done in $N \log N$ time too.

# HankelTransform with fixed circular acceptance angle (circular aperture) for Time of Flight SESANS
#==============================================================================
#acceptq is the q-space needed to create limited acceptance effect
SElength= wavelength*magfield
Copy link
Contributor

Choose a reason for hiding this comment

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

@rmdalgliesh Mentioned that there are some difficulties with specifying things in terms of the magnetic field, as this is not as general as it can be.

# data.has_z_acceptance
return [q]

def transform(data, q_calc, Iq_calc, qmono, Iq_mono):
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably want to keep this under user control to some extent.

Wim Bouwman (w.g.bouwman@tudelft.nl), June 2013
"""

from __future__ import division
Copy link
Contributor

Choose a reason for hiding this comment

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

The future is here!

from __future__ import division

import numpy as np # type: ignore
from numpy import pi, exp # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: should really use np.pi and np.exp


from sas.sascalc.data_util.nxsunit import Converter
wavelength = Converter(wavelength[1])(wavelength[0],"A")
thickness = Converter(thickness[1])(thickness[0],"A")
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these up to date with the latest unit conversion stuff?

If the instrument has a circular acceptance, 1 all_q vector is needed

"""
if not data.has_no_finite_acceptance:
Copy link
Contributor

Choose a reason for hiding this comment

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

is .has_no_finite_acceptance an attribute of all data classes - and should it be?

q_calc, Iq_calc)

def call_HankelAccept(data, q_calc, Iq_calc, q_mono, Iq_mono):
return hankel(data.x, data.lam * 1e-9,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is calling hankel here correct?

q_calc, Iq_calc)

def call_Cosine2D(data, q_calc, Iq_calc, qx, qy, Iq_mono):
return hankel(data.x, data.y, data.lam * 1e-9,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is calling hankel here correct, either?

@smk78
Copy link
Contributor

smk78 commented Nov 7, 2022

@rmdalgliesh commented:

My other concern is the need for a reference for the cosine transform. The hankel transform is well established and references but I'm not aware of anything where the cosine transform for anisotropic scattering is specifically discussed.

@wimbouwman replies by email:

The cosine transform is one of the first SESANS articles (1996) by Theo Rekveldt (https://doi.org/10.1016/0168-583X(96)00213-3). The numerical drawback of the cosine transform is that it is 2D. Only later we found out that it can be written as the Hankeltransform (2008, https://dx.doi.org/10.1107/S0021889808026770), which is only 1D.

@butlerpd
Copy link
Member

@caitwolf , @dehoni @rmdalgliesh @gnsmith @wimbouwman - Should this be a priority for 6.0?

allIq=np.linspace() # This is the model applied to the allq q-space. Needs to refference the model somehow
alldq = (allq[1]-allq[0])*1e10
sigma[i]=wavelength[i]^2*thickness/2/pi*np.sum(allIq*allq*alldq)
s[i]=1-exp(-sigma)
Copy link
Contributor

Choose a reason for hiding this comment

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

sigma[i] and s[i] not used further, would bring the code in line with the other calls to calculate the correlation function G.


G *= dq*1e10*2*pi

P = exp(thickness*wavelength**2/(4*pi**2)*(G-G[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

One would expect to get corrected data in reduced data (spin-echolength, correlation function G). Separate the conversion to polarisation into separate function to avoid code repetition. I would assume that the data format will develop with time and instrument specific information (wavelength, sample thickness, magnetic field) is retained in the data reduction.

@wimbouwman
Copy link
Contributor

wimbouwman commented Nov 27, 2023 via email

@wimbouwman
Copy link
Contributor

wimbouwman commented Nov 27, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants