-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Consolidate typing for coords with Coords and StrongCoords aliases (fixes #7972) #7987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
pymc/distributions/shape_utils.py
Outdated
| # Lazy import to break circular dependency | ||
| if model is None: | ||
| from pymc.model.core import modelcontext | ||
|
|
||
| model = modelcontext(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this functional change? Doesn't seem necessary or a good idea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i introduced the lazy import only to work around the circular import that surfaced during the typing consolidation (shape_utils -> model -> distributions -> shape_utils). I agree that changing runtime behavior here is not ideal
if you prefer, i can revert this functional change and instead resolve the circular dependency purely by relocating the typing aliases to a more independent module
pymc/distributions/shape_utils.py
Outdated
| CoordValue: TypeAlias = Sequence[Hashable] | np.ndarray | None | ||
| Coords: TypeAlias = Mapping[str, CoordValue] | ||
|
|
||
| StrongCoordValue: TypeAlias = tuple[Hashable, ...] | None | ||
| StrongCoords: TypeAlias = Mapping[str, StrongCoordValue] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put these in a more independent place to avoid the new circular import issues. Coordinates are not really distribution specific anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can move CoordValue, Coords, StrongCoordValue, and StrongCoords to a more independent location and update downstream imports accordingly. Let me know if you have a preferred target module for these.
this PR addresses issue #7972 by consolidating the typing of
coordsacross the PyMC codebase.Previously,
coordsappeared with a mix of types such as:SequenceSequence | np.ndarraydict[str, Any]this PR introduces explicit and consistent type aliases:
CoordValueCoordsStrongCoordValueStrongCoords(similar to the existing
Shape,Dims, andStrongDimsaliases inshape_utils)Key changes
Model.coordsto returnStrongCoordsStrongCoordsinstead ofdict[str, Any}Sequence,Sequence | np.ndarray, and untyped dictionariesNotes
while testing this change, a runtime circular import involving:
pymc.model.corepymc.distributions.shape_utilsfrom pymc.model import modelcontextpymc.model(partially initialized)was encountered.
Fixes #7972