pymc-devs / pymc

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

List more math function in API docs #7211

Closed brandonhorsley closed 3 months ago

brandonhorsley commented 3 months ago

Description

This commit is a solution to the problems presented in PyMC github issue #7130. The first part of the issue was that certain functions like inverse trigonometric functions didn't seem to work, however on a clean install for this commit and pull request, that issue disappeared and the functions work as intended. These functions were missing among others from the website documentation which is the second part of the github issue and is what this particular git commit concerns. Note I have included tround although other codes reflect this as being deprecated but still able to be used. I have also rearranged the ordering to better reflect similar functions being bunched together.

Related Issue

Checklist

Type of change


šŸ“š Documentation preview šŸ“š: https://pymc--7211.org.readthedocs.build/en/7211/

welcome[bot] commented 3 months ago

Thank You Banner] :sparkling_heart: Thanks for opening this pull request! :sparkling_heart: 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.

brandonhorsley commented 3 months ago

Doc build worked as expected which is good news, does read a bit long so maybe further polish could be doing further category subdivisions like 'Trig functions', 'tensor functions'...etc but this would probably warrant discussion on what good subcategories could be

jessegrabowski commented 3 months ago

Do all of these functions need to be exposed? I don't think the _numpy functions need to be there, but also the linear algebra routines.

OriolAbril commented 3 months ago

Do all of these functions need to be exposed? I don't think the _numpy functions need to be there, but also the linear algebra routines.

If we expect them to be used they should be somewhere. I am under the impression it is the case, but if it were preferred to use them from pytensor then they can be removed from here and listed on pytensor only.

jessegrabowski commented 3 months ago

My only strong objection is to the numpy ones, because we shouldn't be even suggesting that users use numpy functions on pytensor objects. But I guess this PR is just updating the docs to reflect what functions are already there, so maybe this concern should go in a different thread.

OriolAbril commented 3 months ago

There is the underlying issue that it isn't really clear what is PyMC's public API, even to team members. But I think this is a good place to discuss that for these functions. Any functions we don't think are public should NOT go into the docs

brandonhorsley commented 3 months ago

Yeah I mean with the pull request I have put forward I just covered all the functions presented in the math.py file just out of sheer completeness since I didn't want to make that executive decision without consultation. I believe I also included 'tround' in the math.rst file for this pull request which is listed as deprecated in math.py but does still work so that one would be another one to possibly put onto the cutting room floor since all it does is raise a deprecation warning and call 'round' anyway

ricardoV94 commented 3 months ago

Let's just show the PyTensor functions and not list the numpy ones. Further refining can be done later.

brandonhorsley commented 3 months ago

Is that something to do on my end? If so is it worth omitting the deprecated 'tround' from the list as well or just the numpy ones?

ricardoV94 commented 3 months ago

You can actually remove the deprecated tround and yes omit the numpy ones

ricardoV94 commented 3 months ago

I actually meant removing the tround code as well

brandonhorsley commented 3 months ago

Ahh I'll do that now, remove the numpy code from math.py too?

ricardoV94 commented 3 months ago

I don't know if the numpy code is being used.

Regarding tround, you can remove that code. There's also a round wrapper that can be removed and the module changed to provide access to pytensor.tensor.round directly

ricardoV94 commented 3 months ago

If you want (totally optional), you can deprecate the _numpy functions for now with a FutureWarning. I see that one of them is being used in a test (that is not just testing the function!), but we can just define it there then, no need to be part of our public modules

ricardoV94 commented 3 months ago

@brandonhorsley I pushed a commit that refactors a bit the test code to be more readable. Otherwise I think all is good. Tests are running now

codecov[bot] commented 3 months ago

Codecov Report

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

Project coverage is 91.89%. Comparing base (6252d2e) to head (43ec2ed). Report is 30 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/pymc-devs/pymc/pull/7211/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/7211?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 #7211 +/- ## ========================================== - Coverage 92.23% 91.89% -0.35% ========================================== Files 100 100 Lines 16889 16884 -5 ========================================== - Hits 15578 15515 -63 - Misses 1311 1369 +58 ``` | [Files](https://app.codecov.io/gh/pymc-devs/pymc/pull/7211?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs) | Coverage Ī” | | |---|---|---| | [pymc/math.py](https://app.codecov.io/gh/pymc-devs/pymc/pull/7211?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs#diff-cHltYy9tYXRoLnB5) | `77.71% <100.00%> (+1.82%)` | :arrow_up: | ... and [28 files with indirect coverage changes](https://app.codecov.io/gh/pymc-devs/pymc/pull/7211/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pymc-devs)
welcome[bot] commented 3 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:

ricardoV94 commented 3 months ago

Thanks @brandonhorsley

brandonhorsley commented 3 months ago

Thanks for your help as well @ricardoV94 et al., you've been a big help for getting this first pull request under my belt!

ricardoV94 commented 3 months ago

You're welcome. Looking for your second if that happens!