XYE filewriter #417
XYE filewriter #417
Conversation
| @@ -41,6 +41,7 @@ Features | |||
| ~~~~~~~~ | |||
|
|
|||
| * Coordinate transformation kernel for :math:`\vec{Q}` `412 <https://github.com/scipp/scipp/pull/412>`_. | |||
| * Added :func:`scippneutron.io.xye.save_xye` and :func:`scippneutron.io.xye.load_xye` `417 <https://github.com/scipp/scipp/pull/417>`_. | |||
There was a problem hiding this comment.
The xye module feels like overkill? Will there be more than 2 user-facing functions?
There was a problem hiding this comment.
Probably not. But there may be many functions for different formats. I think it is cleaner to keep them in separate modules.
Note that it is possible to call them with scn.io.save_xye. But Sphinx doesn't want to generate link to that, only to the actual definition of the function.
There was a problem hiding this comment.
We have the same problem in Scipp and elsewhere. I think we have to fix that (maybe by not using automodule, but listing contents by hand?).
| 'Cannot save data to XYE file because it is not one-dimensional. ' | ||
| f'It has dimensions {da.dims}') | ||
| if len(da.coords) == 0: | ||
| raise ValueError('Cannot save data to XYE file because it has no coordinates.') |
There was a problem hiding this comment.
Have you considered raising when there are masks? Dropping them is almost always wrong, so it might avoid hours/days of debugging for users.
There was a problem hiding this comment.
I considered several approaches:
- Raise when there are masks, attrs, other coords and add a flag to ignore this check.
- Log a message about other metadata.
- Do nothing.
I went with the last because I was unsure how useful this check would be. But I agree that masks may cause problems. I can add a check for those.
And maybe a log message of the form 'saving data or unit {unit} with coordiante {coord} to XYE file {fname}'?
There was a problem hiding this comment.
I think ignoring other coords and attrs is not harmful, but ignoring masks definitely is?
src/scippneutron/io/xye.py
Outdated
| if coord is None: | ||
| if len(da.coords) > 1: | ||
| raise ValueError( | ||
| 'Cannot deduce which coordinate to save because the data has more ' | ||
| f'than one: {list(da.coords.keys())}') | ||
| coord = next(iter(da.coords)) |
There was a problem hiding this comment.
This does not feel very convenient. Why not use da.coords[da.dim] by default?
There was a problem hiding this comment.
This seemed risky. Since the file doesn't really include metadata, it is hard to tell what it actually contains.
There was a problem hiding this comment.
But the dimension-coord should be the most sensible one in 99% of cases?
| coord = dim if coord is None else coord | ||
| loaded = np.loadtxt(fname, delimiter=' ', unpack=True) | ||
| if loaded.ndim == 1: | ||
| loaded = loaded[:, np.newaxis] |
There was a problem hiding this comment.
I presume the header is not standardized, so we can make no attempt to read units and dims from there?
There was a problem hiding this comment.
Correct. Mantid writes a similar header but places the units in separate lines.
Fixes #413
easyDiffraction can load files written by
save_xye.