Skip to content

BUG: basic.icdf creates tensor variable of type of the distribution, rather than probability type (float64?) #6648

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

Closed
gokuld opened this issue Apr 3, 2023 · 4 comments · Fixed by #6669
Labels
Milestone

Comments

@gokuld
Copy link
Contributor

gokuld commented Apr 3, 2023

Describe the issue:

In basic.py, we have this definition of the icdf helper function:

def icdf(rv: TensorVariable, value: TensorLike, **kwargs) -> TensorVariable:
    """Create a graph for the inverse CDF of a  Random Variable."""
    value = pt.as_tensor_variable(value, dtype=rv.dtype)
    try:
        return _icdf_helper(rv, value, **kwargs)
    except NotImplementedError:
        # Try to rewrite rv
        fgraph, rv_values, _ = construct_ir_fgraph({rv: value})
        [ir_rv] = fgraph.outputs
        return _icdf_helper(ir_rv, value, **kwargs)

I specifically have a concern about value = pt.as_tensor_variable(value, dtype=rv.dtype)
Should not the input type to icdf function be a floating point type? (Type of probability value).
rv.dtype can be integer too, such as for discrete distributions and is not the right type for the probability value input to the icdf function.

Reproduceable code example:

import pytensor as pt
import pymc as pm
from pymc.logprob.basic import icdf
from pymc.pytensorf import inputvars

dist = pm.Geometric.dist(p=0.1)
dist_icdf = icdf(dist, dist.type())
dist_icdf_fn = pt.function(list(inputvars(dist_icdf)), dist_icdf)
dist_icdf_fn(0.1)

Error message:

TensorType(int64, ()) cannot store accurately value 0.1, it would be represented as 0. If you do not mind this precision loss, you can: 1) explicitly convert your data to a numpy array of dtype int64, or 2) set "allow_input_downcast=True" when calling "function".

PyMC version information:

pytensor: 2.10.1
Python: 3.10.10
PyMC: latest commit b7764dd

Context for the issue:

I would like to implement a util function for a self consistency test for ICDF and logcdf of discrete distributions.

That is, to check if:
value = icdf(dist, exp(logcdf(dist, value)))

@gokuld gokuld added the bug label Apr 3, 2023
@ricardoV94
Copy link
Member

ricardoV94 commented Apr 4, 2023

You're totally right, I think it was a mistake on my part. What we want is the icdf.dtype (the output) to be the same as the rv

@iykat
Copy link
Contributor

iykat commented Apr 4, 2023

@gokuld would you like to assign this issue to me? I will be happy to work on it.

@gokuld
Copy link
Contributor Author

gokuld commented Apr 4, 2023

Sure.

@ricardoV94 ricardoV94 added this to the 5.2.1 milestone Apr 7, 2023
@ricardoV94
Copy link
Member

@iykat since this issue is blocking the next release I am going ahead and fix it.

Apologies if you started working already, and please feel free to pick other beginner friendly issues :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants