fix: defer to pint application registry#198
Merged
Merged
Conversation
dspeed was creating its own pint UnitRegistry and globally overriding the application registry on import via set_application_registry(). This silently hijacked consumer code that also used pint, and produced cross-registry errors when consumers created Quantities against pint's default (or their own) registry before dspeed was imported. Use pint.get_application_registry() instead so dspeed shares whatever registry the consumer is using, with no global side effects on import. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
gipert
force-pushed
the
fix-pint-app-registry
branch
from
1672393 to
8b84bd2
Compare
4 tasks
With dspeed now sharing the application pint registry (instead of
hijacking it), lgdo's preferred short format (~P) propagates: unit
attributes are written as 'ns' instead of 'nanosecond', and pint emits
a DeprecationWarning when user format specs lack a unit formatter.
- Update test_proc_chain_where to expect 'ns' in lh5 attrs.
- Filter the pint DeprecationWarning so user-provided legend format
strings without unit formatters keep working.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(vis): strip units from legend values before format
WaveformBrowser passed pint Quantity objects directly into user-supplied
format strings like 'energy = {trapEmax:0.1f}'. With a registry default
format that includes a unit formatter (~P, set by lgdo), pint emits a
DeprecationWarning predicting that '0.1f' will silently start appending
units in a future release — the opposite of what dspeed users want here.
Convert Quantity legend values to their magnitudes before format(),
preserving the numeric formatting the user requested. Drop the
filterwarnings workaround that hid this warning.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
gipert
force-pushed
the
fix-pint-app-registry
branch
from
7d5d91c to
d71092c
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes dspeed to use Pint’s application registry instead of creating and installing its own registry, avoiding global registry hijacking for downstream consumers.
Changes:
- Replaces dspeed’s private
UnitRegistrysetup withpint.get_application_registry(). - Updates processing-chain unit expectations from canonical names to abbreviated unit strings.
- Adjusts waveform-browser legend formatting to unwrap Pint quantities before string formatting.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/dspeed/units.py |
Uses Pint’s application registry as dspeed’s shared unit registry. |
src/dspeed/vis/waveform_browser.py |
Converts legend Quantity values to magnitudes before formatting. |
tests/test_processing_chain.py |
Updates expected output unit strings from nanosecond to ns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The previous fix stripped magnitudes from every Quantity in leg_dat, but the default-spec branch in WaveformBrowser.__init__ substitutes '0.3gP~' for entries without an explicit format. Stripping there made the format call ValueError out because '0.3gP~' is invalid for plain floats. Parse each legend format string and only strip Quantities whose spec has no pint formatter character (~, P, H, L, C, D). Default '0.3gP~' and explicit pint specs keep the Quantity; user-provided plain specs like '0.1f' get the magnitude so pint's DeprecationWarning does not fire. Also add tests/test_units.py to lock in the application-registry sharing contract (importing dspeed must not replace pint's app registry, and consumer-created quantities must interoperate with dspeed.units.unit_registry quantities). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
Thanks Luigi, this is an important fix, which was causing quite some issues (but I couldnt understand why) |
build_processing_chain stored values via 'processors[key] = node'. When processors is a dbetto AttrsDict (which CLI configs are, after Props.read_from), __setitem__ wraps any plain dict in a fresh AttrsDict via __init__'s copy loop, decoupling the stored entry from the local reference. Later in-place mutations like node["prereqs"] = ... then never reached the entry in processors[key], so resolve_dependencies failed with KeyError: 'prereqs'. Cast processors to a plain dict once at the top of the function so all subsequent assignments behave normally, regardless of the input type. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
gipert
force-pushed
the
fix-pint-app-registry
branch
from
075962a to
9a136c3
Compare
dspeed assumes short-form unit names (e.g. 'ns' rather than 'nanosecond') in output attrs and elsewhere. Today this works because lgdo.units sets formatter.default_format = "~P" on the application registry when lh5 imports it. Make the dependency explicit so dspeed does not silently break if that transitive import side effect goes away or if a consumer uses dspeed.units without triggering lh5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dspeed/units.pywas creating its ownpint.UnitRegistryand globally overriding the application registry on import viaset_application_registry(). This silently hijacked any consumer code that also used pint.pint.Quantity(...)against the default (or their own) registry before importing dspeed, mixing those quantities with dspeed's producedCannot operate with Quantity of different registrieserrors.pint.get_application_registry()so dspeed shares whatever registry the consumer is using, with no global side effects on import.The
auto_reduce_dimensions=Trueflag previously set on the private registry is dropped — it's purely cosmetic (e.g. simplifiesm·s/s → mafter arithmetic) and nothing in dspeed depends on it.Test plan
pint.Quantity(1, "us")and dspeed'sureg.Quantity(1, "us")share the same registry; arithmetic between them works.pint.get_application_registry()is no longer mutated by importing dspeed.tests/processors/test_iir_filter.py,tests/test_build_dsp.pyexerciseunit_registry).🤖 Generated with Claude Code