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 #7093

Closed
wants to merge 0 commits into from
Closed

Rectify return type hints in logprob module rewrites #7093

wants to merge 0 commits into from

Conversation

AryanNanda17
Copy link
Contributor

@AryanNanda17 AryanNanda17 commented Jan 9, 2024

Description

Related Issue

Checklist

Type of change

  • Other (please specify):
def find_measurable_comparisons( 
    fgraph: FunctionGraph, node: Node 
) -> Optional[List[MeasurableComparison]]: 

Here, the return type hint is wrong. 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--7093.org.readthedocs.build/en/7093/

Copy link

welcome bot commented Jan 9, 2024

Thank You Banner
💖 Thanks for opening this pull request! 💖 The PyMC community really appreciates your time and effort to contribute to the project. Please make sure you have read our Contributing Guidelines and filled in our pull request template to the best of your ability.

@AryanNanda17
Copy link
Contributor Author

@larryshamalama please have a look at the pull request.

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Sorry for the delay in reviewing

@ricardoV94 ricardoV94 changed the title Rectified Return type hint#6811 Rectify return type hints in logprob module rewrites Jan 11, 2024
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7bb2ccd) 92.21% compared to head (4c73bd3) 76.34%.
Report is 18 commits behind head on main.

❗ Current head 4c73bd3 differs from pull request most recent head fd3ced5. Consider uploading reports for the commit fd3ced5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #7093       +/-   ##
===========================================
- Coverage   92.21%   76.34%   -15.87%     
===========================================
  Files         101      101               
  Lines       16912    16889       -23     
===========================================
- Hits        15595    12894     -2701     
- Misses       1317     3995     +2678     
Files Coverage Δ
pymc/logprob/binary.py 29.54% <100.00%> (-66.93%) ⬇️
pymc/logprob/censoring.py 61.29% <100.00%> (-37.59%) ⬇️
pymc/logprob/checks.py 42.59% <100.00%> (-57.41%) ⬇️
pymc/logprob/cumsum.py 88.23% <100.00%> (-11.77%) ⬇️
pymc/logprob/tensor.py 48.76% <100.00%> (-30.38%) ⬇️

... and 67 files with indirect coverage changes

@AryanNanda17
Copy link
Contributor Author

AryanNanda17 commented Jan 11, 2024

@ricardoV94 , In this file:-
pymc/pymc/logprob/tensor.py

 def find_measurable_stacks( 
     fgraph, node 
 ) -> Optional[List[Union[TensorVariable, TensorVariable]]]:

This went wrong. What is wrong here?

@ricardoV94
Copy link
Member

@ricardoV94 , In this file:- pymc/pymc/logprob/tensor.py

 def find_measurable_stacks( 
     fgraph, node 
 ) -> Optional[List[Union[TensorVariable, TensorVariable]]]:

This went wrong. What is wrong here?

The Union is not needed, since you have the same return type now

@AryanNanda17
Copy link
Contributor Author

I updated it.
Thanks!

@AryanNanda17
Copy link
Contributor Author

Screenshot 2024-01-12 at 2 08 57 AM

Is this a formatting issue?
@ricardoV94

@AryanNanda17
Copy link
Contributor Author

AryanNanda17 commented Jan 15, 2024

@ricardoV94 @larryshamalama Please look into the issue. I researched a lot during the past few days, but did not get anything specific about the error here.
Thanks

@ricardoV94
Copy link
Member

@AryanNanda17 yes that's a formatting issue due to pre-commit. You can read about it in our contributing guides: https://www.pymc.io/projects/docs/en/latest/contributing/pr_tutorial.html

Check the pre-commit instructions specifically

@AryanNanda17
Copy link
Contributor Author

@ricardoV94, I made the changes and now the precommits tests are passing.
Screenshot 2024-01-31 at 12 32 57 AM

@AryanNanda17
Copy link
Contributor Author

AryanNanda17 commented Jan 30, 2024

@ricardoV94 Why am I getting errors during mergeable check?

@larryshamalama
Copy link
Member

@ricardoV94 Why am I getting errors during mergeable check?

Hi @AryanNanda17, sorry about such delays in replying - I am pushing through graduate studies, which have been time-consuming. I will look more carefully at this afternoon or tomorrow.

Have you:

  • Rebased with respect to main? Currently, I see that this branch has conflicts. This is probably due to the addition of commits to the main branch
  • Applied pre-commit? Our linter and pre-commit configurations recently changed (#7901)

@AryanNanda17
Copy link
Contributor Author

My feature branch was some commits behind the main branch and I was trying to resolve merge conflicts, by mistake I discarded all the commits, and the pull request got closed, really sorry.
I will make a new pull request.

@larryshamalama
Copy link
Member

larryshamalama commented Feb 1, 2024

Sounds good, do not hesitate to ping me in the new PR once you will have done so.

Edit: FYI, this was previously worked on in #7013. In principle, priority is given to preceding PRs, but it was my fault for providing late reviews. If possible, co-authorship will be given regardless of the merged PR (I will look into how to do this)

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

Successfully merging this pull request may close these issues.

3 participants