-
Notifications
You must be signed in to change notification settings - Fork 1
Implement Perrichon2022 McStas TOSCA function as lookup table #43
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
base: main
Are you sure you want to change the base?
Conversation
RastislavTuranyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments, mostly to do with the most recent changes from #39, but there are two major things that stick out:
- No docstrings for
get_kernelandget_peak - No unit tests
Could you add these? That said, tho, not sure how viable unit tests are in this case, ig they'll just have to be reference values to test for regression.
|
|
||
| from .model_base import InstrumentModel, ModelData | ||
| from .mixins import SimpleBroaden1DMixin | ||
| from ..instrument import INSTRUMENT_DATA_PATH |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure i like this circular import (i'm surprised it works) - should we move it elsewhere?
| from pathlib import Path | ||
|
|
||
| from numpy.polynomial import Polynomial | ||
| from scipy.interpolate import RegularGridInterpolator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to import these here instead of the top of the file?
| self.polynomial = Polynomial(coef=self.data["coef"], | ||
| domain=self.data["domain"], | ||
| window=self.data["window"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how nitpicky to be, but this is not aligned... 😅
| def get_kernel(self, | ||
| omega_q: Float[np.ndarray, 'sample dimension=1'], | ||
| mesh: Float[np.ndarray, 'mesh'], | ||
| ) -> Float[np.ndarray, 'sample mesh']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def get_kernel(self, | |
| omega_q: Float[np.ndarray, 'sample dimension=1'], | |
| mesh: Float[np.ndarray, 'mesh'], | |
| ) -> Float[np.ndarray, 'sample mesh']: | |
| def get_kernel(self, | |
| points: Float[np.ndarray, 'sample dimension=1'], | |
| mesh: Float[np.ndarray, 'mesh'], | |
| ) -> Float[np.ndarray, 'sample mesh']: |
Also, could you add a docstring?
| assert len(omega_q.shape) == 2 and omega_q.shape[1] == 1 | ||
| energy = omega_q |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| assert len(omega_q.shape) == 2 and omega_q.shape[1] == 1 | |
| energy = omega_q | |
| assert points.ndim == 2 and points.shape[1] == 1 | |
| energy = points |
First, related to the discussion #39, is this check necessary? Secondly, are you sure you want to keep it as a 2D array?
| def get_peak(self, | ||
| omega_q: Float[np.ndarray, 'sample dimension=1'], | ||
| mesh: Float[np.ndarray, 'mesh'], | ||
| ) -> Float[np.ndarray, 'sample mesh']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| def get_peak(self, | |
| omega_q: Float[np.ndarray, 'sample dimension=1'], | |
| mesh: Float[np.ndarray, 'mesh'], | |
| ) -> Float[np.ndarray, 'sample mesh']: | |
| def get_peak(self, | |
| points: Float[np.ndarray, 'sample dimension=1'], | |
| mesh: Float[np.ndarray, 'mesh'], | |
| ) -> Float[np.ndarray, 'sample mesh']: |
Again, could you add a docstring please?
| omega_q: Float[np.ndarray, 'sample dimension=1'], | ||
| mesh: Float[np.ndarray, 'mesh'], | ||
| ) -> Float[np.ndarray, 'sample mesh']: | ||
| shifted_meshes = [mesh - energy for energy in omega_q[:, 0]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| shifted_meshes = [mesh - energy for energy in omega_q[:, 0]] | |
| try: | |
| points = points[:, 0] | |
| except IndexError as e: | |
| raise InvalidPointsError( | |
| f'The provided array of points (shape={points.shape}) is not valid. The points ' | |
| f'array must be a Nx1 2D array where N is the number of energy transfers.' | |
| ) from e | |
| shifted_meshes = [mesh - energy for energy in points] |
| shifted_kernels = [ | ||
| self.get_kernel(np.array([omega_q]), shifted_mesh) | ||
| for omega_q, shifted_mesh in zip(omega_q, shifted_meshes) | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| shifted_kernels = [ | |
| self.get_kernel(np.array([omega_q]), shifted_mesh) | |
| for omega_q, shifted_mesh in zip(omega_q, shifted_meshes) | |
| ] | |
| shifted_kernels = [ | |
| self.get_kernel(np.array([[point]]), shifted_mesh) | |
| for point, shifted_mesh in zip(points, shifted_meshes) | |
| ] |
This is an interpolated lookup approach to the TOSCA resolution function, using McStas data from Adrien Perrichon

This scheme is a "scaled" lookup table:
The table is quite compact but still a bit chunky to include in the .yaml files. Instead we store all the model parameters in a .npz file and point to this from the .yaml.
Some pre-processing was performed on the original data: