Change load_with_mantid and from_mantid to return data groups #444
Conversation
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "sample = raw_sample['data']\n", |
There was a problem hiding this comment.
Have you considers naming this detectors instead of data?
There was a problem hiding this comment.
I haven't. Why detectors? Does this match the nexus structure more closely?
There was a problem hiding this comment.
Yes. On the other hand there may be workspaces that do not correspond to detector data.
There was a problem hiding this comment.
Do you still think it should be renamed?
There was a problem hiding this comment.
Not sure. What do you think?
There was a problem hiding this comment.
This will never produce the same nested structure as a typical NeXus file. So I don't see this as a strong argument. Plus 'data' is shorter than 'detector'. But it is also very generic (there are a lot of things one could call data).
So I'm leaning towards leaving it as is but I'm not opposed to changing it. Not very helpful, I know...
| if 'position' in res["data"].coords: | ||
| res["data"].coords['position'] = res["data"].coords['position'][spec_dim, 0] | ||
| res["data"] = res["data"][spec_dim, 0].copy() | ||
| return res |
There was a problem hiding this comment.
Is there so much duplication in this file because you copied the code for being able to preserve the old version in parallel? It makes it very hard to review, as it appears as completely new code.
There was a problem hiding this comment.
Essentially, yes. We touch this code so little that I didn't see the value in avoiding the duplication. It would require making each function handle both cases or reimplementing the old one on top of the new one.
Or do you see a better option.
The main differences between the new and old functions are how they build data arrays / data groups from coords_labs_data.
There was a problem hiding this comment.
Any recommendation on how to review this?
SimonHeybrock
left a comment
There was a problem hiding this comment.
Reviewed mantid.py based on diff without the deprecated content, LGTM.
|
|
||
| def _wrap_dict_in_variable(d: Dict[str, Any]) -> Dict[str, sc.Variable]: | ||
| return { | ||
| key: val if isinstance(val, sc.Variable) else sc.scalar(val) |
There was a problem hiding this comment.
This is like https://github.com/scipp/scippnexus/blob/99850ab2dbf3ef156db623bd7200be7d9b3332bc/src/scippnexus/v2/base.py#L22-L23, maybe name similarly? The function name also implies that the entire dict is wrapped in a (single) variable.
No description provided.