Skip to content
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

Simplify Model __new__ and metaclass #7473

Merged
merged 14 commits into from
Oct 10, 2024

Commits on Oct 10, 2024

  1. Type get_context correctly

    get_context returns an instance of a Model, not a ContextMeta object
    We don't need the typevar, since we don't use it for anything special
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    ddf6f10 View commit details
    Browse the repository at this point in the history
  2. Import from future to use delayed evaluation of annotations

    All of these are supported on python>=3.9.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    74f6616 View commit details
    Browse the repository at this point in the history
  3. New ModelManager class for managing model contexts

    We create a global instance of it within this module, which is similar
    to how it worked before, where a `context_class` attribute was attached
    to the Model class.
    
    We inherit from threading.local to ensure thread safety when working
    with models on multiple threads. See pymc-devs#1552 for the reasoning. This is
    already tested in `test_thread_safety`.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    aa253ac View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    d5beee6 View commit details
    Browse the repository at this point in the history
  5. Fix type of UNSET in type definition

    UNSET is the instance of the _UnsetType type.
    We should be typing the latter here.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    e2cefc3 View commit details
    Browse the repository at this point in the history
  6. Set model parent in init rather than in __new__

    We use the new ModelManager.parent_context property to reliably set any
    parent context, or else set it to None.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    5a1f0b8 View commit details
    Browse the repository at this point in the history
  7. Replace get_context in metaclass with classmethod

    We set this directly on the class as a classmethod, which is clearer
    than going via the metaclass.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    8846db9 View commit details
    Browse the repository at this point in the history
  8. Remove get_contexts from metaclass

    The original function does not behave as I expected.
    In the following example I expected that it would return only the final
    model, not root.
    
    This method is not used anywhere in the pymc codebase, so I have dropped
    it from the codebase. I originally included the following code to replace
    it, but since it is not used anyway, it is better to remove it.
    
    ```python`
    @classmethod
    def get_contexts(cls) -> list[Model]:
        """Return a list of the currently active model contexts."""
        return MODEL_MANAGER.active_contexts
    ```
    
    Example for testing behaviour in current main branch:
    ```python
    import pymc as pm
    
    with pm.Model(name="root") as root:
        print([c.name for c in pm.Model.get_contexts()])
        with pm.Model(name="first") as first:
            print([c.name for c in pm.Model.get_contexts()])
        with pm.Model(name="m_with_model_None", model=None) as m_with_model_None:
            # This one doesn't make much sense:
            print([c.name for c in pm.Model.get_contexts()])
    ```
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    7318d75 View commit details
    Browse the repository at this point in the history
  9. Simplify ContextMeta

    We only keep the __call__ method, which is necessary to keep the
    model context itself active during that model's __init__.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    560cd3b View commit details
    Browse the repository at this point in the history
  10. Type Model.register_rv for for downstream typing

    In pymc/distributions/distribution.py, this change allows the type
    checker to infer that `rv_out` can only be a TensorVariable.
    
    Thanks to @ricardoV94 for type hint on rv_var.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    6ff3d5d View commit details
    Browse the repository at this point in the history
  11. Include np.ndarray as possible type for coord values

    I originally tried numpy's ArrayLike, replacing Sequence entirely, but then I realized
    that ArrayLike also allows non-sequences like integers and floats.
    
    I am not certain if `values="a string"` should be legal. With the type hint sequence, it is.
    Might be more accurate, but verbose to use `list | tuple | set | np.ndarray | None`.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    2ebc48f View commit details
    Browse the repository at this point in the history
  12. Use function-scoped new_dims to handle type hint varying throughout f…

    …unction
    
    We don't want to allow the user to pass a `dims=[None, None]` to our function, but current behaviour
    set `dims=[None] * N` at the end of `determine_coords`.
    
    To handle this, I created a `new_dims` with a larger type scope which matches
    the return type of `dims` in `determine_coords`.
    
    Then I did the same within def Data to support this new type hint.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    165a35f View commit details
    Browse the repository at this point in the history
  13. Fix case of dims = [None, None, ...]

    The only case where dims=[None, ...] is when the user has passed dims=None. Since the user passed dims=None,
    they shouldn't be expecting any coords to match that dimension. Thus we don't need to try to add any
    more coords to the model.
    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    dbbb9a2 View commit details
    Browse the repository at this point in the history
  14. Remove unused hack

    thomasaarholt committed Oct 10, 2024
    Configuration menu
    Copy the full SHA
    8565965 View commit details
    Browse the repository at this point in the history