-
Notifications
You must be signed in to change notification settings - Fork 11
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
Crystal metadata change #843
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
9592db2
add the class and a test
stan-dot f707b51
add to the beamline files
stan-dot 103e177
Fix the expected test spacing values
stan-dot 0aee3ea
Implement crystal metadata for I03 DCM (#870)
rtuck99 11871a6
make d spacing a signal
stan-dot edd9fef
delete the absent keys test
stan-dot f2e8920
remove the d spacing calculation
stan-dot 060784d
respond to feedback
stan-dot b489f57
fix crystal metadata variable namign
stan-dot be6aee6
convert the types correctly
stan-dot 961aa70
ruff
stan-dot 749bf8a
apparently this fixes tests and coverage
stan-dot cb9abcf
remove print
stan-dot d3e79cd
remove is None checks
stan-dot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import math | ||
from dataclasses import dataclass | ||
from enum import Enum | ||
from typing import Literal | ||
|
||
|
||
@dataclass(frozen=True) | ||
class Material: | ||
""" | ||
Class representing a crystalline material with a specific lattice parameter. | ||
""" | ||
|
||
name: str | ||
lattice_parameter: float # Lattice parameter in meters | ||
|
||
|
||
class MaterialsEnum(Enum): | ||
Si = Material(name="silicon", lattice_parameter=5.4310205e-10) | ||
Ge = Material(name="germanium", lattice_parameter=5.6575e-10) | ||
|
||
|
||
@dataclass(frozen=True) | ||
class CrystalMetadata: | ||
""" | ||
Metadata used in the NeXus format, | ||
see https://manual.nexusformat.org/classes/base_classes/NXcrystal.html | ||
""" | ||
|
||
usage: Literal["Bragg", "Laue"] | ||
type: str | ||
reflection: tuple[int, int, int] | ||
d_spacing: tuple[float, str] | ||
|
||
@staticmethod | ||
def calculate_default_d_spacing( | ||
lattice_parameter: float, reflection: tuple[int, int, int] | ||
) -> tuple[float, str]: | ||
""" | ||
Calculates the d-spacing value in nanometers based on the given lattice parameter and reflection indices. | ||
""" | ||
h_index, k_index, l_index = reflection | ||
stan-dot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
d_spacing_m = lattice_parameter / math.sqrt( | ||
h_index**2 + k_index**2 + l_index**2 | ||
) | ||
d_spacing_nm = d_spacing_m * 1e9 # Convert meters to nanometers | ||
return round(d_spacing_nm, 5), "nm" | ||
|
||
|
||
def make_crystal_metadata_from_material( | ||
material: MaterialsEnum, | ||
reflection_plane: tuple[int, int, int], | ||
usage: Literal["Bragg", "Laue"] = "Bragg", | ||
d_spacing_param: tuple[float, str] | None = None, | ||
): | ||
d_spacing = d_spacing_param or CrystalMetadata.calculate_default_d_spacing( | ||
material.value.lattice_parameter, reflection_plane | ||
) | ||
assert all( | ||
isinstance(i, int) and i > 0 for i in reflection_plane | ||
), "Reflection plane indices must be positive integers" | ||
return CrystalMetadata(usage, material.value.name, reflection_plane, d_spacing) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import pytest | ||
|
||
from dodal.common.crystal_metadata import ( | ||
MaterialsEnum, | ||
make_crystal_metadata_from_material, | ||
) | ||
|
||
|
||
def test_happy_path_silicon(): | ||
crystal_metadata = make_crystal_metadata_from_material(MaterialsEnum.Si, (3, 1, 1)) | ||
|
||
# Check the values | ||
assert crystal_metadata.type == "silicon" | ||
assert crystal_metadata.reflection == (3, 1, 1) | ||
assert crystal_metadata.d_spacing == pytest.approx( | ||
(0.16375, "nm"), rel=1e-3 | ||
) # Allow for small tolerance | ||
assert crystal_metadata.usage == "Bragg" | ||
|
||
|
||
def test_happy_path_germanium(): | ||
crystal_metadata = make_crystal_metadata_from_material(MaterialsEnum.Ge, (1, 1, 1)) | ||
# Check the values | ||
assert crystal_metadata.type == "germanium" | ||
assert crystal_metadata.reflection == (1, 1, 1) | ||
assert crystal_metadata.d_spacing == pytest.approx( | ||
(0.326633, "nm"), rel=1e-3 | ||
) # Allow for small tolerance | ||
assert crystal_metadata.usage == "Bragg" | ||
|
||
|
||
def test_invalid_reflection_plane_with_negative_number(): | ||
with pytest.raises( | ||
AssertionError, | ||
match="Reflection plane indices must be positive integers", | ||
): | ||
make_crystal_metadata_from_material(MaterialsEnum.Si, (-1, 2, 3)) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should: Sorry, maybe I've misunderstood, I thought the conclusion of the discussion at #843 (comment) was to remove all this calculation and use the PV? Or is it not yet on i22?
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.
If the PV isn't on i22 yet then I said in that comment, can we get the ball rolling on that?
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.
there was a discussion was to add the PV on i18, which ultimately just was a slight name change.
regarding the static method
at the moment there is the PV value used. I might have mistaken with the line above where the 'initial value' is used.
at the moment the only place when the calculation is used is in the
make_crystal_metadata_from_material
where if the d_spacing is not supplied by the user, it's calculated.we could remove it and make it required
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.
Ok, so we still need it in there for i22. That's fine, I'll make a note on the conversion issue.