Skip to content

Allow empty slice when loading log with sublogs#299

Merged
jl-wynen merged 2 commits intomainfrom
allow-empty-slice
May 5, 2026
Merged

Allow empty slice when loading log with sublogs#299
jl-wynen merged 2 commits intomainfrom
allow-empty-slice

Conversation

@jl-wynen
Copy link
Copy Markdown
Member

@jl-wynen jl-wynen commented May 1, 2026

Loading an NXlog that contians sublogs with x[:] raises an exception because we don't support slicing sublogs. But in this case, it should behave the same as x[()] or x[...] for 1D data. So this PR handles it this way.

I ran into this in ESSreduce when passing the default TimeInterval (slice(None, None, None)) to a loader for an NXlog.

@jl-wynen jl-wynen requested a review from SimonHeybrock May 1, 2026 09:18
Comment thread src/scippnexus/_common.py Outdated
if isinstance(select, tuple) and len(select) == 0:
return {}
if select == slice(None, None, None):
check_1d()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is the branch below that does elif isinstance(select, int | sc.Variable) or isinstance(select, slice): not sufficient?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Because that returns a 1d slice which leads to this error downstream:

     59 def read_children(self, sel: ScippIndex) -> sc.DataGroup:
     60     # Sublogs have distinct time axes (with a different length). Must disable
     61     # positional indexing.
     62     if self._sublogs and ('time' in to_canonical_select(list(self.sizes), sel)):
---> 63         raise sc.DimensionError(
     64             "Cannot positionally select time since there are multiple "
     65             "time fields. Label-based selection is not supported yet."
     66         )
     67     dg = super().read_children(sel)
     68     for name, field in self._sublog_children.items():

DimensionError: Cannot positionally select time since there are multiple time fields. Label-based selection is not supported yet.

def read_children(self, sel: ScippIndex) -> sc.DataGroup:
# Sublogs have distinct time axes (with a different length). Must disable
# positional indexing.
if self._sublogs and ('time' in to_canonical_select(list(self.sizes), sel)):
raise sc.DimensionError(
"Cannot positionally select time since there are multiple "
"time fields. Label-based selection is not supported yet."
)
dg = super().read_children(sel)
for name, field in self._sublog_children.items():
dg[name] = field[sel]
return dg

Alternatively, we could change the check here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, so this specifically fixes a bug when the ESS-style "sublogs" are present (which are not in the standard, iirc)? A more targeted fix might indeed make sense, but I don't mind too much.

Can you update the PR title/description?

I think this also means that the new tests do not really do much that didn't work before, as they contain no sublogs as far as I can see. The removed line in test_log_with_connection_status_raises_with_positional_and_label_indexing is I think the test you actually need, except that it would now not raise.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated

@jl-wynen jl-wynen changed the title Allow empty slice select Allow empty slice when loading log with sublogs May 5, 2026
@jl-wynen jl-wynen force-pushed the allow-empty-slice branch from 16102e9 to 19fafcd Compare May 5, 2026 07:42
@jl-wynen jl-wynen merged commit 3550be5 into main May 5, 2026
5 checks passed
@jl-wynen jl-wynen deleted the allow-empty-slice branch May 5, 2026 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants