Skip to content

[BUG] correctly set tags of Hurdle and Truncated depending on inner distribution#569

Merged
fkiraly merged 7 commits into
mainfrom
hurdle-tags
Aug 2, 2025
Merged

[BUG] correctly set tags of Hurdle and Truncated depending on inner distribution#569
fkiraly merged 7 commits into
mainfrom
hurdle-tags

Conversation

@fkiraly
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly commented Jul 28, 2025

This PR correctly sets the tag of Hurdle and Truncated depending on the inner distribution.

Related to #566

@fkiraly fkiraly added the module:probability&simulation probability distributions and simulators label Jul 28, 2025
@fkiraly fkiraly changed the title [ENH] correctly set tags of Hurdle and Truncated depending on inner distribution [BUG] correctly set tags of Hurdle and Truncated depending on inner distribution Jul 28, 2025
@tingiskhan
Copy link
Copy Markdown
Contributor

Hmm, this seems really strange:

image

Why is it testing the PMF for a truncated Normal? I think this was the issue I had earlier as well, when I used the wrong "measure" tag.

@tingiskhan
Copy link
Copy Markdown
Contributor

I think that this might be the issue:

image

I think we need to remove the method names that aren't relevant when changing measuretype.

@tingiskhan
Copy link
Copy Markdown
Contributor

@fkiraly , see #571

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Aug 1, 2025

see #571, continuous distributions should return 0 for pmf - this should actually be the default?

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Aug 2, 2025

The "one-off" error from #566 is still present. Any ideas?

@tingiskhan
Copy link
Copy Markdown
Contributor

Hm, very strange - I see that it tests Poisson for Truncated, I thought I changed all Poisson references to Negative Binomial. What happens if you replace it with NegativeBinomial instead?

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Aug 2, 2025

that would not fix the error, just "make the tests pass"?

@tingiskhan
Copy link
Copy Markdown
Contributor

I'll debug and see what's wrong!

@tingiskhan
Copy link
Copy Markdown
Contributor

tingiskhan commented Aug 2, 2025

Can't seem to reproduce the error:

import tqdm

from skpro.distributions import Normal, TruncatedDistribution, Hurdle
import numpy as np


def ppf_and_cdf(d):
    capabilities_exact = d.get_tags()["capabilities:exact"]

    if "ppf" not in capabilities_exact or "cdf" not in capabilities_exact:
        return
    x = d.sample()
    x_approx = d.ppf(d.cdf(x))
    if d.ndim > 0:
        assert np.allclose(x.values, x_approx.values)
    else:
        assert np.allclose(x, x_approx)


cases = TruncatedDistribution.get_test_params()

for i in tqdm.tqdm(range(1_000)):
    for case in cases:
        dist = TruncatedDistribution(**case)
        ppf_and_cdf(dist)

Tried bumping to 5,000, still no issue. I'm on scipy==1.16.0 and Python=3.12. Will try to update scipy if see if that's the issue.

Edit: No issue

@tingiskhan
Copy link
Copy Markdown
Contributor

Perhaps we should move the eps fix to truncated instead?

@tingiskhan
Copy link
Copy Markdown
Contributor

@fkiraly, what happens if you merge the changes of #576?

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Aug 2, 2025

ok, I will try to merge #576 into this and then we see

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Aug 2, 2025

I think there is a genuine bug in #576, unrelated to the fix. What I will do, to make debugging easier: I will merge this without #576. and then you can run check_estimator locally to fix the distribution - does this make sense?

This reverts commit ecca172.
@tingiskhan
Copy link
Copy Markdown
Contributor

Sounds good. I had messed up which variable to look at when selecting the eps since I assumed that shifted_p would be an array, hence the failing tests

@fkiraly fkiraly merged commit 84ff8fc into main Aug 2, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement module:probability&simulation probability distributions and simulators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants