Skip to content

Check for consistency in source grouping - #766

Merged
takluyver merged 5 commits into
masterfrom
fix/source_group
Nov 5, 2026
Merged

takluyver merged 5 commits into
masterfrom
fix/source_group

Conversation

@daniloefl

@daniloefl daniloefl commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

This fixes issue #765

It was tested as follows:

from extra_data.display import SourceGroup
s = SourceGroup()
print(s.add('SQS_ILH_LAS/PROC/BS_CAM_800_OU1'))
print(s.add('SQS_ILH_LAS/PROC/BS_CAM_800_OU2'))
print(s.add("SQS_ILH_LAS/PROC/BS_CAM_TT_OU2"))

The result is:

True
True
False

Before the fix the result was:

True
True
True

@daniloefl
daniloefl requested a review from takluyver November 4, 2026 11:04
@takluyver

Copy link
Copy Markdown
Member

Thanks! I didn't think the fix was quite right here, though - items can still be in a group if the varying part is numeric in one case but not in another - it just won't be treated as a numeric grouping in that case.

What should have prevented this was a check that it's the same part changing in the new entry as between existing ones. But I had made a mistake which meant that this check only started on the 4th addition to the group. I've pushed a version which fixes this now.

@daniloefl daniloefl changed the title Check for type consistency in source grouping Check for consistency in source grouping Nov 5, 2026
@daniloefl

Copy link
Copy Markdown
Contributor Author

Thanks! I didn't think the fix was quite right here, though - items can still be in a group if the varying part is numeric in one case but not in another - it just won't be treated as a numeric grouping in that case.

What should have prevented this was a check that it's the same part changing in the new entry as between existing ones. But I had made a mistake which meant that this check only started on the 4th addition to the group. I've pushed a version which fixes this now.

Thank you for checking! That is indeed better. With that change, if it looks good for you, I will merge.

@takluyver takluyver added this to the 1.26 milestone Nov 5, 2026
@takluyver

Copy link
Copy Markdown
Member

Thanks, I'm happy with this.

@takluyver
takluyver merged commit ca00f5c into master Nov 5, 2026
10 checks passed
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