Skip to content

Feature/collection validation and python313 - #666

Open
losterbr wants to merge 4 commits into
ladybug-tools:masterfrom
losterbr:feature/collection-validation-and-python313
Open

losterbr wants to merge 4 commits into
ladybug-tools:masterfrom
losterbr:feature/collection-validation-and-python313

Conversation

@losterbr

@losterbr losterbr commented Oct 8, 2026 •

Copy link
Copy Markdown

Summary

Four fixes to the data collection classes, each in its own commit, plus a metadata tweak.

1. Immutable collections could be mutated, and corrupted their source (6015b88)

  • to_mutable() and to_immutable() returned a collection that shared the original's header. Converting units on one relabelled the other. For example, a collection holding Celsius values ended up labelled Fahrenheit.
  • Immutable collections inherited the in-place convert_to_unit, convert_to_ip and convert_to_si, which changed their values. These now raise AttributeError. to_unit(), to_ip() and to_si() return a converted mutable copy, as the module docs describe.
  • HourlyContinuousCollectionImmutable.mutable_class returned DailyCollection. This made get_aligned_collection(mutable=True) raise TypeError. It now returns HourlyContinuousCollection.

2. Validation survives python -O (d917b7c)

Collection value checks used assert, which Python strips under -O. They now raise TypeError or ValueError explicitly.

3. Descriptive errors for no-match filters (0fdc323)

After change 2, empty results raise ValueError, but the three filter wrappers still caught AssertionError. A filter that matched nothing therefore showed the generic "must include at least one value" message. The wrappers now catch ValueError and keep their original messages.

4. Metadata (a82aa91)

Adds the Python 3.13 classifier (is in scope in CI, so assume this is supported)

Behaviour changes to note

  • Exception types. Invalid values now raise TypeError/ValueError, not AssertionError. Filters that match nothing raise ValueError.
  • Headers are copies. to_mutable() and to_immutable() give each collection its own header. Code that relied on the old sharing will no longer see header changes propagate. I found none in this repo, and I haven't checked downstream packages.
  • In-place conversion on immutable collections now raises AttributeError.

Testing

  • 443 tests pass on Python 3.12, and each commit also passes on its own.
  • New tests are parametrised over all five collection types (discontinuous, continuous, daily, monthly, monthly-per-hour). They cover header isolation, the convert_to_* blocking, the to_* copies, mutable_class/immutable_class wiring, get_aligned_collection mutability, and the no-match filter messages.
  • I checked the tests are sensitive by deliberately breaking each fix in a scratch copy. All 25 deliberate breakages were caught.
  • I did not run Python 3.10 or 3.13, or macOS, Linux and Windows. CI covers those.

…ders

to_mutable() and to_immutable() handed the new collection the original's
header, so converting units on one silently relabeled the other. Each
collection now gets its own header copy.

Immutable collections also inherited the in-place convert_to_unit,
convert_to_ip and convert_to_si, which changed their values. These now
raise AttributeError; to_unit, to_ip and to_si convert a mutable copy.

HourlyContinuousCollectionImmutable.mutable_class returned DailyCollection,
which broke get_aligned_collection(mutable=True). It now returns
HourlyContinuousCollection.

Tests cover all five collection types.
Collections now raise ValueError instead of AssertionError when empty, but
the filter wrappers still caught AssertionError. A filter that matched
nothing therefore surfaced the generic "must include at least one value"
message instead of explaining which condition or range matched nothing.

filter_by_conditional_statement, filter_by_range and
filter_collections_by_statement now catch ValueError and raise it with
their original messages.
@CLAassistant

CLAassistant commented Oct 8, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

This branch has not been deployed

No deployments
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