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

Parameterization of WeibullRV missing scale? #1141

Closed
Armavica opened this issue Aug 23, 2022 · 5 comments
Closed

Parameterization of WeibullRV missing scale? #1141

Armavica opened this issue Aug 23, 2022 · 5 comments

Comments

@Armavica
Copy link

Armavica commented Aug 23, 2022

Description of your problem or feature request

I am trying to upgrade PyMC to aesara=2.8.2, and some of the tests failing are related to WeibullRV.

I think that it might have to do with commit fcd1c4c which imposes the prototype of WeibullRV.__call__(self, shape, size, **kwargs). However, the Weibull distribution is usually described by two parameters: its scale and its shape. The scale is missing from the function prototype, which might explain the errors on PyMC's side.

cc @rlouf because I think that you are the author of this commit

Please provide a minimal, self-contained, and reproducible example.

import pymc as pm # (branch Armavica:aesara-rewrite)
with pm.Model() as model:
    pm.Weibull("x", alpha=1.0, beta=1.0, shape=(3, 4))

Please provide the full traceback of any errors.

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Input In [3], in <cell line: 1>()
      1 with pm.Model() as model:
----> 2     pm.Weibull("x", alpha=1.0, beta=1.0, size=(3, 4))

File ~/src/pymc/pymc/distributions/distribution.py:263, in Distribution.__new__(cls, name, rng, dims, initval, observed, total_size, transform, *args, **kwargs)
    259     raise TypeError(f"Name needs to be a string but got: {name}")
    261 # Create the RV and process dims and observed to determine
    262 # a shape by which the created RV may need to be resized.
--> 263 rv_out, dims, observed, resize_shape = _make_rv_and_resize_shape(
    264     cls=cls, dims=dims, model=model, observed=observed, args=args, **kwargs
    265 )
    267 if resize_shape:
    268     # A batch size was specified through `dims`, or implied by `observed`.
    269     rv_out = change_rv_size(rv=rv_out, new_size=resize_shape, expand=True)

File ~/src/pymc/pymc/distributions/distribution.py:165, in _make_rv_and_resize_shape(cls, dims, model, observed, args, **kwargs)
    162 """Creates the RV and processes dims or observed to determine a resize shape."""
    163 # Create the RV without dims information, because that's not something tracked at the Aesara level.
    164 # If necessary we'll later replicate to a different size implied by already known dims.
--> 165 rv_out = cls.dist(*args, **kwargs)
    166 ndim_actual = rv_out.ndim
    167 resize_shape = None

File ~/src/pymc/pymc/distributions/continuous.py:2585, in Weibull.dist(cls, alpha, beta, *args, **kwargs)
   2582 alpha = at.as_tensor_variable(floatX(alpha))
   2583 beta = at.as_tensor_variable(floatX(beta))
-> 2585 return super().dist([alpha, beta], *args, **kwargs)

File ~/src/pymc/pymc/distributions/distribution.py:351, in Distribution.dist(cls, dist_params, shape, **kwargs)
    346 create_size, ndim_expected, ndim_batch, ndim_supp = find_size(
    347     shape=shape, size=size, ndim_supp=cls.rv_op.ndim_supp
    348 )
    349 # Create the RV with a `size` right away.
    350 # This is not necessarily the final result.
--> 351 rv_out = cls.rv_op(*dist_params, size=create_size, **kwargs)
    353 # Replicate dimensions may be prepended via a shape with Ellipsis as the last element:
    354 if shape is not None and Ellipsis in shape:

TypeError: WeibullRV.__call__() got multiple values for argument 'size'

Please provide any additional information below.

Versions and main components

  • Aesara version: 2.8.2
  • Aesara config (python -c "import aesara; print(aesara.config)")
  • Python version: 3.10.5
  • Operating system: linux
  • How did you install Aesara: conda
@rlouf
Copy link
Member

rlouf commented Aug 23, 2022

Did you open an issue in PyMC?

@Armavica
Copy link
Author

Armavica commented Aug 23, 2022

Here is my draft PR: pymc-devs/pymc#6059.

Sorry if I was unclear, but the issue is not in the current version of PyMC. It appeared when I tried to update aesara's version to 2.8.2. WeibullRV is called by PyMC with two parameters, and this worked fine until aesara<2.8.0, but now it looks like it wants only one. Since the recent release notes of aesara don't mention anything particular related to the API of WeibullRV (only docstrings), I assumed that this change was unintentional.

@rlouf
Copy link
Member

rlouf commented Aug 23, 2022

Thanks for linking the relevant PR. The idea behind the change in the WeibullRV API was to conform to the API in NumPy. It's easy enough to rescale if necessary.

@Armavica
Copy link
Author

Thank you, that answers my question. In this case, I will close this issue as I think that this should probably be treated on the PyMC side.

@rlouf
Copy link
Member

rlouf commented Aug 24, 2022

It looks like the parametrization change was propagated to AePPL as well, so this should not be an issue.

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

No branches or pull requests

2 participants