Skip to content

State label membership by having no value - #950

Merged
d-chambers merged 4 commits into
devfrom
label-membership
Aug 20, 2026
Merged

State label membership by having no value#950
d-chambers merged 4 commits into
devfrom
label-membership

Conversation

@d-chambers

@d-chambers d-chambers commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

An inventory label (and an annotation) used to say "this interval is in group X" with a boolean value: noisy,true. That made the group's shape — membership, which may overlap, versus function, which may not — depend on the Python type of its values, and left false as a label that validated, serialized, and then projected to nothing (_fill_from_intervals only ever set True). It also needed a custom serializer on OpticalPathLabel to keep a value of 1 from being pruned as equal to the default True.

This PR makes membership a matter of having no value:

  • OpticalPathLabel.value and Annotation.value are str | int | float | None, default None. A label with no value states membership of its group; a label with a value states what the group holds there.
  • true/false are refused everywhere a value enters — normalize_value (both models), and _parse_labels in the directory loader with the file and row named — with a message saying membership is spelled by leaving the value empty.
  • value_kind(None) == "membership" replaces the "boolean" kind; _fill_from_intervals unions membership intervals unconditionally. A group which states membership in one row and holds a value in another is a kind mix and is refused, in both the inventory and an AnnotationSet.
  • The _write_object_type override and _wanted helper on OpticalPathLabel are deleted; with a default of None, nothing collides with 1.

The inventory tutorial gains a "The shape of a track" section stating the three interval shapes (tiling / function / membership), what an uncovered channel gets for each, that numeric groups project as float (absence is NaN), the one-name-one-coordinate rule, and the half-open-with-run-end fencepost projection applies. Its labels CSV now writes 150,300,noisy,.

Projection and selection are unchanged: a membership group still projects as a bool coordinate, spool.select(noisy=True) / expand_by("noisy") behave as before.

Follow-ups from a six-way adversarial review (Codex + five lenses):

  • AnnotationSet CSV writer builds each column as an object series, so an int beside an unset cell is written as 5, not 5.0 (pandas inferred a float column from the None). Note that pandas does the same inference when constructing a frame from a plain list, so an in-memory frame mixing ints with unset rows should pass dtype=object; a table read back is already an object column.
  • A membership-only set (an all-blank value column) now reloads equal to what was saved; the reader had inferred a string dtype from nothing.
  • The directory loader reads a whitespace-only value cell as blank, since a blank cell is now load-bearing.
  • The refusal messages say what a false row should become — deleted, not blanked — since blanking it would turn an exclusion into membership. The annotation-side refusal names the group.
  • A valued annotation group with a blank cell is now refused as a kind mix (previously such a row silently read back as True); the message says a blank cell states membership.
  • Three Keep an annotation value of 1 through serialization #912 tests that guarded the deleted serializer override are removed; they asserted stock pydantic behaviour.

Changelog

  • changed breaking: OpticalPathLabel and Annotation values may no longer be booleans; a label with no value states membership of its group, and true/false cells are refused with a message saying so.
  • changed: An OpticalPathLabel or Annotation with no value now reads back with value=None rather than True.
  • fixed: A label with value=false no longer validates and then silently projects nothing.
  • fixed: An AnnotationSet written to CSV keeps an integer value an integer when another row of the column is unset.

Checklist

I have:

  • filled in the Changelog section above (see docs/contributing/general_guidelines.qmd).

I have (if applicable):

  • referenced the GitHub issue this PR closes.
  • documented the new feature with docstrings and/or appropriate doc page.
  • included tests. See testing guidelines.
  • added the "ready_for_review" tag once the PR is ready to be reviewed.

A label or annotation with a boolean value made a group's shape depend
on its values' Python type, and left `false` as a label which validated
and then projected nothing. Membership is now stated by carrying no
value: `value` is `str | int | float | None`, default None, and
true/false are refused by the value normalizer and the directory loader.
The `1 == True` serializer workaround on OpticalPathLabel is no longer
needed and is removed. The inventory tutorial gains a section stating
the three interval shapes and the rules which follow from them.
@d-chambers d-chambers added the ready_for_review PR is ready for review label Aug 20, 2026
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@d-chambers, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 527f6bde-7b12-4d2d-8d3f-768534983ed5

📥 Commits

Reviewing files that changed from the base of the PR and between 4bc33ba and bb0845a.

📒 Files selected for processing (12)
  • dascore/core/_spool_inventory.py
  • dascore/core/annotations.py
  • dascore/core/inventory.py
  • dascore/core/inventory_loader.py
  • dascore/utils/intervals.py
  • docs/tutorial/inventory.qmd
  • tests/test_core/test_annotation_loader.py
  • tests/test_core/test_annotations.py
  • tests/test_core/test_inventory.py
  • tests/test_core/test_inventory_loader.py
  • tests/test_proc/test_proc_inventory.py
  • tests/test_utils/test_intervals.py

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@d-chambers d-chambers added the documentation Improvements or additions to documentation label Aug 20, 2026
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (4bc33ba) to head (bb0845a).

Additional details and impacted files
@@            Coverage Diff            @@
##               dev      #950   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          195       195           
  Lines        24891     24895    +4     
=========================================
+ Hits         24891     24895    +4     
Flag Coverage Δ
network 44.64% <23.52%> (-0.02%) ⬇️
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown

✅ Documentation built:
👉 Download
Note: You must be logged in to github and a DASDAE member to access the link.

The annotation CSV writer builds each column as an object series, so
an int beside an unset cell is no longer written as a float; an
all-blank value column reloads with one dtype so a membership-only
set round-trips equal; the directory loader reads a whitespace-only
value cell as blank. The refusal messages now say a false row is
deleted rather than blanked, and name the annotation group. Three
tests guarding the deleted serializer override are removed.
@d-chambers
d-chambers merged commit c299c74 into dev Aug 20, 2026
30 checks passed
@d-chambers
d-chambers deleted the label-membership branch August 20, 2026 18:24
d-chambers added a commit that referenced this pull request Aug 21, 2026
true and false as values. The inventory plots were written before that
and merged after it, so each branch was green against its own base and
the pair broke on dev: a group which states membership loads from CSV
into a string column, where its None reads back as NaN, and the
renderer handed that to normalize_value, which refuses what is not
finite. The inventory tutorial stopped building on that line.

The renderer now takes #950's vocabulary rather than working around it.
A row states membership by holding no value, whichever of None, NaN or
NA the column's dtype produces; such a lane takes one color and is
named by itself. Booleans are no longer a kind here either, since they
are no longer a kind anywhere. A number missing from a numeric lane
draws in the color which says nothing was stated, rather than raising.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ready_for_review PR is ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant