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

List more math function in API docs #7211

Merged
merged 10 commits into from
Mar 27, 2024

Conversation

brandonhorsley
Copy link
Contributor

@brandonhorsley brandonhorsley commented Mar 22, 2024

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

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

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

…ithub issue pymc-devs#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.
Copy link

welcome bot commented Mar 22, 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.

@brandonhorsley
Copy link
Contributor Author

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

@brandonhorsley brandonhorsley marked this pull request as ready for review March 22, 2024 13:33
@ricardoV94 ricardoV94 changed the title This commit is a partial solution to the problems presented in PyMC g… List more math function in API docs Mar 22, 2024
Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Everything looks good, I agree with sorting them into multiple topic related subsections but can be done in a follow up PR

@jessegrabowski
Copy link
Member

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

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

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

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

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

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

@brandonhorsley
Copy link
Contributor Author

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

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

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 25, 2024

I actually meant removing the tround code as well

@brandonhorsley
Copy link
Contributor Author

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

@ricardoV94
Copy link
Member

ricardoV94 commented Mar 25, 2024

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

ricardoV94 commented Mar 25, 2024

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

Removed tround, replaced round with pytensor.tensor import, and added FutureWarning to log1mexp_numpy and logdiffexp_numpy.
pymc/math.py Outdated Show resolved Hide resolved
pymc/math.py Outdated Show resolved Hide resolved
brandonhorsley and others added 3 commits March 25, 2024 11:20
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Co-authored-by: Ricardo Vieira <28983449+ricardoV94@users.noreply.github.com>
Searched PyMC codebase for occurrences of _numpy functions and added
explicit warnings. Most of these were for log1mexp_numpy which had quite
a few involvements in test codes. Also note that on test_math.py the
_numpy functions are explicitly imported and so when deprecated those
imports will have to be removed too.
tests/distributions/test_continuous.py Outdated Show resolved Hide resolved
tests/test_math.py Outdated Show resolved Hide resolved
brandonhorsley and others added 3 commits March 25, 2024 13:34
This commit undoes the changes to test_math.py by removing the
FutureWarning warnings for the _numpy functions. Note that when these
tests are removed the _numpy functions are imported explicitly in this
file. Replaced the log1mexp_numpy use in test_continuous.py by just
substituting in the maths from the function to avoid calling the
function.
@ricardoV94
Copy link
Member

@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

Copy link

codecov bot commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

@@            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 Coverage Δ
pymc/math.py 77.71% <100.00%> (+1.82%) ⬆️

... and 28 files with indirect coverage changes

@ricardoV94 ricardoV94 merged commit 81d31c8 into pymc-devs:main Mar 27, 2024
23 checks passed
Copy link

welcome bot commented Mar 27, 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 🎁

@ricardoV94
Copy link
Member

Thanks @brandonhorsley

@brandonhorsley
Copy link
Contributor Author

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

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

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

Successfully merging this pull request may close these issues.

ENH: Expansion of pymc.math to include more numpy/pytensor functions
4 participants