Skip to content

Add a CIF writer #89

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.

Sign up for GitHub

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

Merged
jl-wynen merged 13 commits into from
Sep 19, 2024
Merged

Add a CIF writer #89

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a47473a
Fix __all__ in __init__.py
jl-wynen Sep 3, 2024
3dd094c
Remove unused ficture
jl-wynen Sep 3, 2024
882c8e4
Begin CIF writer
jl-wynen Sep 3, 2024
665ad91
Use block.add_reduced_powder_data
jl-wynen Sep 5, 2024
100f390
Use new scippneutron cif builder
jl-wynen Sep 5, 2024
ed07543
Require scippneutron>=24.9.0
jl-wynen Sep 19, 2024
89cf198
Add a comment to the saved file
jl-wynen Sep 19, 2024
35b9937
Use correct return type
jl-wynen Sep 19, 2024
3fbdc65
Fix package version
jl-wynen Sep 19, 2024
0a4f969
Apply ruff fixed
jl-wynen Sep 19, 2024
ff2186e
Import from correct module
jl-wynen Sep 19, 2024
0550457
Fix tests to use new CIF interface
jl-wynen Sep 19, 2024
8380d27
Check orcid id in cif
jl-wynen Sep 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions tests/dream/geant4_reduction_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@
import sciline
import scipp as sc
import scipp.testing
from scippneutron.io.cif import Author

import ess.dream.data # noqa: F401
from ess import dream, powder
from ess.dream.io.cif import CIFAuthor, CIFAuthors
from ess.powder.types import (
AccumulatedProtonCharge,
BackgroundRun,
CalibrationFilename,
CIFAuthors,
DspacingBins,
Filename,
IofDspacing,
Expand All @@ -24,7 +25,7 @@
NeXusSample,
NeXusSource,
NormalizedByProtonCharge,
OutFilename,
ReducedDspacingCIF,
SampleRun,
TofMask,
TwoThetaBins,
Expand Down Expand Up @@ -57,8 +58,8 @@
WavelengthMask: None,
CIFAuthors: CIFAuthors(
[
CIFAuthor(
name="Jane Doe", email="[email protected]", id_orcid="0000-0000-0000-0001"
Author(
Copy link
Member

@YooSunYoung YooSunYoung Sep 19, 2024

Choose a reason for hiding this comment

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

Would it be better to add one more Author for testing since it's CIFAuthors...?

Copy link
Member Author

@jl-wynen jl-wynen Sep 19, 2024

Choose a reason for hiding this comment

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

I don't think this is so important. The tests here are mainly about checking that the workflow routes the correct data to the writer. The writer itself is tested in ScippNeutron.

Using multiple authors here makes testing harder because then we have a loop over authors in the file. And I already find the string-based tests brittle. So I would like to avoid testing a multi line string for a loop.

name="Jane Doe", email="[email protected]", orcid="0000-0000-0000-0001"
),
]
),
Expand Down Expand Up @@ -159,26 +160,19 @@ def test_use_workflow_helper(workflow):


def test_pipeline_can_save_data(workflow):
def get_result(da: IofDspacing) -> IofDspacing:
return da

buffer = io.StringIO()
workflow[OutFilename] = buffer
workflow = powder.with_pixel_mask_filenames(workflow, [])
result = workflow.compute(ReducedDspacingCIF)

result, expected = workflow.bind_and_call(
[dream.io.save_reduced_dspacing, get_result]
)
sc.testing.assert_identical(result, expected)

buffer = io.StringIO()
result.save(buffer)
buffer.seek(0)
content = buffer.read()
# print(content)
# assert False

assert content.startswith(r'#\#CIF_1.1')
_assert_contains_source_info(content)
_assert_contains_author_info(content)
_assert_contains_beamline_info(content)
_assert_contains_dspacing_data(content)


def _assert_contains_source_info(cif_content: str) -> None:
Expand All @@ -188,3 +182,14 @@ def _assert_contains_source_info(cif_content: str) -> None:
def _assert_contains_author_info(cif_content: str) -> None:
assert "audit_contact_author.name 'Jane Doe'" in cif_content
assert 'audit_contact_author.email [email protected]' in cif_content
Copy link
Member

@YooSunYoung YooSunYoung Sep 19, 2024

Choose a reason for hiding this comment

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

Orcid ID could be tested as well.

Copy link
Member Author

@jl-wynen jl-wynen Sep 19, 2024

Choose a reason for hiding this comment

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

I didn't want to replicate all tests from ScippNeutron. But I can add this.

Copy link
Member

@YooSunYoung YooSunYoung Sep 19, 2024

Choose a reason for hiding this comment

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

Ah ha, right it's essdiffraction specific test...



def _assert_contains_beamline_info(cif_content: str) -> None:
assert 'diffrn_source.beamline DREAM' in cif_content
assert 'diffrn_source.facility ESS' in cif_content


def _assert_contains_dspacing_data(cif_content: str) -> None:
assert 'pd_proc.d_spacing' in cif_content
assert 'pd_proc.intensity_net' in cif_content
assert 'pd_proc.intensity_net_su' in cif_content