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.

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

fix: remove non-finite values #101

Merged
merged 2 commits into from
Oct 24, 2024
Merged

fix: remove non-finite values #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b078062
fix: remove non-finite values
jokasimr Oct 24, 2024
0cf0c30
test: nonfinite values are filtered from ORSO
jokasimr Oct 24, 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
7 changes: 3 additions & 4 deletions src/ess/amor/orso.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,10 @@ def build_orso_iofq_dataset(
r = sc.values(iofq.data)
sr = sc.stddevs(iofq.data)
sqz = sigma_q.to(unit="1/angstrom", copy=False)
data = (qz, r, sr, sqz)

return OrsoIofQDataset(
OrsoDataset(header, np.column_stack([_extract_values_array(d) for d in data]))
)
data = np.column_stack(tuple(map(_extract_values_array, (qz, r, sr, sqz))))
data = data[np.isfinite(data).all(axis=-1)]
return OrsoIofQDataset(OrsoDataset(header, data))


def _extract_values_array(var: sc.Variable) -> np.ndarray:
Expand Down
6 changes: 6 additions & 0 deletions tests/amor/pipeline_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,19 @@ def test_run_data_pipeline(amor_pipeline: sciline.Pipeline):
@pytest.mark.filterwarnings("ignore:Invalid transformation, missing attribute")
def test_run_full_pipeline(amor_pipeline: sciline.Pipeline):
amor_pipeline[SampleRotation[SampleRun]] = sc.scalar(0.85, unit="deg")
# Make the Q range cover a larger interval than the experiment is sensitive to.
# This let's us test the non-covered regions are filtered from the ORSO data.
amor_pipeline[QBins] = sc.geomspace(
dim="Q", start=0.005, stop=0.15, num=391, unit="1/angstrom"
)
Comment on lines +72 to +76
Copy link
Member

Choose a reason for hiding this comment

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

Would be nicer to put this in a separate test, instead of having one "let us test every aspect" one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. But that would require re-running the pipeline again, making the tests slower.
So for that reason I opted for making it part of the test for the ORSO output instead.

Copy link
Member

Choose a reason for hiding this comment

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

Since we both agree, maybe we can live with the slower tests? Alternatively, write a unit test for just this function, without running the full pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we both agree, maybe we can live with the slower tests?

That's a weird way of putting it. I agreed about the aspect you brought up. But taking into account the drawback mentioned I opted for another solution.

If you prefer a different trade-off, for example having slower but more well separated tests, we can do that instead.

amor_pipeline[Filename[SampleRun]] = amor.data.amor_sample_run(608)
res = amor_pipeline.compute(orso.OrsoIofQDataset)
assert res.info.data_source.experiment.instrument == "Amor"
assert res.info.reduction.software.name == "ess.reflectometry"
assert res.data.ndim == 2
assert res.data.shape[1] == 4
assert np.all(res.data[:, 1] >= 0)
assert np.isfinite(res.data).all()


@pytest.mark.filterwarnings("ignore:Failed to convert .* into a transformation")
Expand Down