Ymir imaging I/O helpers #38

YooSunYoung · 2024-08-28T14:46:29Z

Related to #34

Reference: https://github.com/scipp/ess-legacy/blob/683747138f1775d106809bf17bf5f56b75d6e0d3/imaging/helper_funcs.py#L41-L68

TODO

I think open beam and dark current are the other way around.
Add test data and tests
Add documentation
Add integration test ?

SimonHeybrock

Is this for some non-compliant Nexus files, or why can't we use ScippNexus?

SimonHeybrock · 2024-09-02T04:00:08Z

src/ess/imaging/io.py

+from pathlib import Path
+from typing import NewType
+
+import jax.numpy as np


Missing in project dependencies, or is this unintentional?

Yes it was unintentional...

SimonHeybrock · 2024-09-02T04:01:15Z

src/ess/imaging/io.py

+        ]
+        x_length, y_length = detector_dataset["value"].shape[1:]
+        return HistogramModeDetectorData(
+            sc.sort(


I think such a sort can be quite costly. Is this required?

We need to slice the data based on the time so it should be sorted along the time.
I think the data is already almost sorted in most cases.

SimonHeybrock · 2024-09-02T04:02:51Z

src/ess/imaging/io.py

+    image_detector_name: ImageDetectorName,
+    histogram_mode_detectors_path: HistogramModeDetectorsPath = DEFAULT_HISTOGRAM_PATH,
+) -> HistogramModeDetectorData:
+    with File(file_path, mode="r") as f:


Please use h5py.File for clarity which File this is.

If this is NeXus, why can't we use ScippNexus?

YooSunYoung · 2024-09-05T10:18:59Z

Is this for some non-compliant Nexus files, or why can't we use ScippNexus?

The 2d detector images are stored in int16, it crashes with scippnexus.
Or is there a way you can skip incompatible part?

YooSunYoung

I used scippnexus wherever I could

YooSunYoung · 2024-09-05T10:20:59Z

src/ess/imaging/io.py

+from pathlib import Path
+from typing import NewType
+
+import jax.numpy as np


Yes it was unintentional...

YooSunYoung · 2024-09-05T10:48:44Z

src/ess/imaging/io.py

+        ]
+        x_length, y_length = detector_dataset["value"].shape[1:]
+        return HistogramModeDetectorData(
+            sc.sort(


We need to slice the data based on the time so it should be sorted along the time.
I think the data is already almost sorted in most cases.

YooSunYoung · 2024-09-05T13:26:13Z

Hmm...! I was wrong. I thought scippnexus raised an error when loading the images but it seems okay. I replaced the h5py to scippnexus.

YooSunYoung · 2024-09-05T13:31:39Z

I just need to update the dependencies to run the CI.

YooSunYoung · 2024-09-10T13:55:57Z

We decided not to use data array to separate images, but the dictionary.
I'll reopen it when it's fixed.

jl-wynen · 2024-09-11T15:13:21Z

src/ess/imaging/io.py

+def _assign_coord_or_make_one(
+    da: sc.DataArray, coord_name: str, coord: sc.Variable | None
+) -> None:
+    da.coords[coord_name] = coord or sc.arange(


This only works when coord is None or a scalar without unit. Otherwise it raises a unit error or dimension error. (Note that coord or ... calls bool(coord))

I fixed it...! Now it makes a dimensionless coordinate only if there is no coordinate.

jl-wynen · 2024-09-11T15:14:46Z

src/ess/imaging/io.py

+    if min_coord is not None and max_coord is not None:
+        return da[dim, min_coord:max_coord]
+    elif min_coord is not None and max_coord is None:
+        return da[dim, min_coord:]
+    elif min_coord is None and max_coord is not None:
+        return da[dim, :max_coord]
+    else:
+        return da


Suggested change

if min_coord is not None and max_coord is not None:

return da[dim, min_coord:max_coord]

elif min_coord is not None and max_coord is None:

return da[dim, min_coord:]

elif min_coord is None and max_coord is not None:

return da[dim, :max_coord]

else:

return da

return da[dim, min_coord:max_coord]

You are reimplementing the behaviour of slice here.

Ah... so I don't have to handle None...? That's great.

I fixed it!

jl-wynen · 2024-09-12T09:51:27Z

src/ess/imaging/io.py

+    if (dim in da.coords.keys()) and (coord is not None):
+        da.coords[dim] = coord
+    elif (dim not in da.coords.keys()) and (coord is None):
+        da.coords[dim] = sc.arange(dim=dim, start=0, stop=da.sizes[dim])


This changed the logic. Now it raises if (dim not in da.coords) and (coord is not None). Previously, it would insert the new coord.

Yes, we don't want a new coordinate if it already exists.
I just wanted to make sure it has a coordinate before slicing.
So, if the loaded data already has a coordinate, you can change the cropping range according to that.

But that contradicts the code. If a coord exists and coord is not None, you overwrite the existing coord. The case I pointed out above would insert a coord if there is non already.

Yeah I added that when we want to insert the coordinate but it should be in the loader function not in here.. I'll remove that part....

src/ess/imaging/io.py

jl-wynen · 2024-09-12T14:16:06Z

src/ess/imaging/io.py

+    sorted_logs = sc.sort(log, 'time')
+    min_time = sc.datetime(da.coords['time'].min().value - 1, unit='ns')
+    max_time = sc.datetime(da.coords['time'].max().value + 1, unit='ns')
+    time_edges = sc.concat((min_time, sorted_logs.coords['time'], max_time), 'time')


This looks fishy. Why are min_time and max_time computed from da.coords['time'] but the edges (except first and last) are constructed from log?

It was because of the out-or-range-values and assumming that logs are always less than the actual time coordinate but I think it can go wrong pretty easily...

I updated it so that it just slice the dataset according to the log.

Co-authored-by: Jan-Lukas Wynen <[email protected]>

SimonHeybrock reviewed Sep 2, 2024

View reviewed changes

YooSunYoung commented Sep 5, 2024

View reviewed changes

YooSunYoung force-pushed the ymir-imaging branch from fe57450 to b677655 Compare September 5, 2024 10:49

YooSunYoung changed the base branch from main to dependency September 6, 2024 08:47

YooSunYoung force-pushed the ymir-imaging branch from 956a954 to 06b50aa Compare September 6, 2024 08:47

YooSunYoung marked this pull request as ready for review September 6, 2024 09:09

Base automatically changed from dependency to main September 10, 2024 07:35

YooSunYoung added 13 commits September 10, 2024 09:35

Back up

897848d

Clean up imaging IO.

3eb405b

Use scippnexus if possible. [skip ci]

4043380

Fix image key based indexing [skip ci]

e5884d3

Add small image dataset for testing [skip ci].

5d6ba28

Add histogram mode detector IO example in the docs. [skip ci]

1095bd3

Update dataset to have integer dtype after reshaping. [skip ci]

ddd5a78

Fix dark current and open beam image key value. [skip ci]

b80671f

Include attributes to copy.

46c50e0

Remove unnecessary loading.

50c1669

Update workflow. [skip ci]

022a782

Added minimum IO tests. [skip ci]

603690c

Manually assign counts unit to the histogram mode detector images.

6f4bc9f

YooSunYoung force-pushed the ymir-imaging branch from e228af4 to 6f4bc9f Compare September 10, 2024 07:35

YooSunYoung marked this pull request as draft September 10, 2024 13:56

YooSunYoung added 2 commits September 10, 2024 16:32

Load/separate image stacks by keys, without binning/grouping.

2bbc2ed

Add explanation why the image keys are treated like this.

2307879

YooSunYoung marked this pull request as ready for review September 10, 2024 14:42

Fix image slicing.

d498454

Add positional coordinate params and cropping params.

6a4596d

YooSunYoung requested a review from jl-wynen September 11, 2024 14:24

jl-wynen reviewed Sep 11, 2024

View reviewed changes

Fix slicing and coordinate assignment.

a96ecf2

YooSunYoung requested a review from jl-wynen September 12, 2024 09:46

jl-wynen reviewed Sep 12, 2024

View reviewed changes

Simplify coordinate assigning logic.

9313b90

jl-wynen reviewed Sep 12, 2024

View reviewed changes

YooSunYoung and others added 5 commits September 13, 2024 14:33

Fix log coordinate broadcasting.

2610708

Skip coordinate deriving

a08fea4

Shallow copy to assign coordinate.

969f223

Update src/ess/imaging/io.py

3cb0306

Co-authored-by: Jan-Lukas Wynen <[email protected]>

Fix if condition.

38c8db6

jl-wynen approved these changes Sep 16, 2024

View reviewed changes

YooSunYoung merged commit 2541c21 into main Sep 16, 2024
4 checks passed

YooSunYoung deleted the ymir-imaging branch September 16, 2024 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ymir imaging I/O helpers #38

Ymir imaging I/O helpers #38

YooSunYoung commented Aug 28, 2024 •

edited

Loading

SimonHeybrock left a comment

SimonHeybrock Sep 2, 2024

YooSunYoung Sep 5, 2024

SimonHeybrock Sep 2, 2024

YooSunYoung Sep 5, 2024

SimonHeybrock Sep 2, 2024

YooSunYoung commented Sep 5, 2024

YooSunYoung left a comment

YooSunYoung Sep 5, 2024

YooSunYoung Sep 5, 2024

YooSunYoung commented Sep 5, 2024

YooSunYoung commented Sep 5, 2024

YooSunYoung commented Sep 10, 2024

jl-wynen Sep 11, 2024

YooSunYoung Sep 12, 2024

jl-wynen Sep 11, 2024

YooSunYoung Sep 12, 2024

YooSunYoung Sep 12, 2024

jl-wynen Sep 12, 2024

YooSunYoung Sep 12, 2024

jl-wynen Sep 12, 2024

YooSunYoung Sep 12, 2024

YooSunYoung Sep 12, 2024

jl-wynen Sep 12, 2024

YooSunYoung Sep 13, 2024

Ymir imaging I/O helpers #38

Ymir imaging I/O helpers #38

Conversation

YooSunYoung commented Aug 28, 2024 • edited Loading

SimonHeybrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooSunYoung commented Sep 5, 2024

YooSunYoung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooSunYoung commented Sep 5, 2024

YooSunYoung commented Sep 5, 2024

YooSunYoung commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YooSunYoung commented Aug 28, 2024 •

edited

Loading