pymc-devs / pymc

Bayesian Modeling and Probabilistic Programming in Python
https://docs.pymc.io/
Other
8.68k stars 2.01k forks source link

Rectify return type hints in logprob module rewrites #7125

Closed AryanNanda17 closed 8 months ago

AryanNanda17 commented 8 months ago

Description

Related Issue

Checklist

Type of change

The return type hint is wrong in many files. MeasurableComparision is an Op, but what is returned are TensorVariables whose node has that Op. This needs to be rectified for all the functions which intend to find a measurable op.


📚 Documentation preview 📚: https://pymc--7125.org.readthedocs.build/en/7125/

AryanNanda17 commented 8 months ago

@larryshamalama and @ricardoV94 please look into this issue.

AryanNanda17 commented 8 months ago

All the pre commits test are passing:-

Screenshot 2024-02-02 at 1 59 55 AM
codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (94020c9) 90.17% compared to head (fb1c48f) 90.20%. Report is 3 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125/graphs/tree.svg?width=650&height=150&src=pr&token=JFuXtOJ4Cb&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) ```diff @@ Coverage Diff @@ ## main #7125 +/- ## ========================================== + Coverage 90.17% 90.20% +0.02% ========================================== Files 101 101 Lines 16932 16961 +29 ========================================== + Hits 15269 15299 +30 + Misses 1663 1662 -1 ``` | [Files](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Δ | | |---|---|---| | [pymc/logprob/binary.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL2JpbmFyeS5weQ==) | `94.31% <100.00%> (+0.06%)` | :arrow_up: | | [pymc/logprob/censoring.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL2NlbnNvcmluZy5weQ==) | `95.69% <100.00%> (+0.04%)` | :arrow_up: | | [pymc/logprob/checks.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL2NoZWNrcy5weQ==) | `90.74% <100.00%> (+0.17%)` | :arrow_up: | | [pymc/logprob/cumsum.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL2N1bXN1bS5weQ==) | `91.17% <100.00%> (+0.26%)` | :arrow_up: | | [pymc/logprob/tensor.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9sb2dwcm9iL3RlbnNvci5weQ==) | `76.85% <100.00%> (+0.19%)` | :arrow_up: | ... and [5 files with indirect coverage changes](https://app.codecov.io/gh/pymc-devs/pymc/pull/7125/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)
AryanNanda17 commented 8 months ago
Screenshot 2024-02-02 at 2 24 46 PM

@larryshamalama and @ricardoV94 why did some tests failed.

It shows this file failed but I didn't even change this file.

ricardoV94 commented 8 months ago

@AryanNanda17 this issue is already been worked by another contributor in https://github.com/pymc-devs/pymc/pull/7013

We will give priority to the first one as the contributor mentioned he still wants to work on it

AryanNanda17 commented 8 months ago

@larryshamalama You could have told me this when I raised this issue #7093 but I kept working on it to figure out what was going wrong.

ricardoV94 commented 8 months ago

Unfortunately, I missed the duplicated work when you opened that first PR. I am sorry.

The info about related PRs should show up in the original issue, but I don't usually check if a PR is already linked when someone opens a new one.

ricardoV94 commented 8 months ago

I see @larryshamalama mentioned an option of adding people as co-authors. We can also do that, specially since this PR is already complete it seems.

I'll check the unrelated failing test. Could you update the title of the PR to the old one?

AryanNanda17 commented 8 months ago

@ricardoV94 any idea why the tests failed?

ricardoV94 commented 8 months ago

It's failing elsewhere, not this PR's fault

AryanNanda17 commented 8 months ago

@ricardoV94 and @larryshamalama thanks for looking into the issue. Let me know if any update is required. Edit - Could you please add old PR labels to this PR?

larryshamalama commented 8 months ago

We're looking into the failed test in #7136

welcome[bot] commented 8 months ago

Congratulations Banner] Congrats on merging your first pull request! :tada: We here at PyMC are proud of you! :sparkling_heart: Thank you so much for your contribution :gift: