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

Rectify return type hints in logprob module rewrites #7125

Merged
merged 2 commits into from
Feb 6, 2024
Merged

Rectify return type hints in logprob module rewrites #7125

merged 2 commits into from
Feb 6, 2024

Conversation

AryanNanda17
Copy link
Contributor

@AryanNanda17 AryanNanda17 commented Feb 1, 2024

Description

Related Issue

Checklist

Type of change

  • Other (please specify):

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
Copy link
Contributor Author

@larryshamalama and @ricardoV94 please look into this issue.

@AryanNanda17
Copy link
Contributor Author

AryanNanda17 commented Feb 1, 2024

All the pre commits test are passing:-
Screenshot 2024-02-02 at 1 59 55 AM

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@            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 Coverage Δ
pymc/logprob/binary.py 94.31% <100.00%> (+0.06%) ⬆️
pymc/logprob/censoring.py 95.69% <100.00%> (+0.04%) ⬆️
pymc/logprob/checks.py 90.74% <100.00%> (+0.17%) ⬆️
pymc/logprob/cumsum.py 91.17% <100.00%> (+0.26%) ⬆️
pymc/logprob/tensor.py 76.85% <100.00%> (+0.19%) ⬆️

... and 5 files with indirect coverage changes

@AryanNanda17
Copy link
Contributor Author

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
Copy link
Member

@AryanNanda17 this issue is already been worked by another contributor in #7013

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

@ricardoV94 ricardoV94 closed this Feb 2, 2024
@AryanNanda17
Copy link
Contributor Author

@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
Copy link
Member

ricardoV94 commented Feb 2, 2024

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
Copy link
Member

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?

@ricardoV94 ricardoV94 reopened this Feb 2, 2024
@AryanNanda17 AryanNanda17 changed the title Aryan#6811 Rectify return type hints in logprob module rewrites Feb 2, 2024
@AryanNanda17
Copy link
Contributor Author

@ricardoV94 any idea why the tests failed?

@ricardoV94
Copy link
Member

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

@AryanNanda17
Copy link
Contributor Author

AryanNanda17 commented Feb 2, 2024

@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
Copy link
Member

We're looking into the failed test in #7136

ricardoV94 and others added 2 commits February 6, 2024 09:55
Co-authored-by: Vivek Anand Singh <17vivekanandsingh@gmail.com>
@ricardoV94 ricardoV94 merged commit 972f3c4 into pymc-devs:main Feb 6, 2024
21 of 23 checks passed
Copy link

welcome bot commented Feb 6, 2024

Congratulations Banner]
Congrats on merging your first pull request! 🎉 We here at PyMC are proud of you! 💖 Thank you so much for your contribution 🎁

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

Successfully merging this pull request may close these issues.

Rectify Return type hint
3 participants