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

improve math rendering of pymc model docs #240

Merged
merged 2 commits into from
Sep 18, 2023

Conversation

jpreszler
Copy link
Contributor

This resolves issue #238
Screenshot 2023-09-16 at 2 25 54 PM

@drbenvincent
Copy link
Collaborator

Not sure why the doctests are failing - they pass when I run then locally. Sometimes remote tests randomly fail and then pass when they are re-run, but that hasn't worked here (yet).

@drbenvincent drbenvincent added the documentation Improvements or additions to documentation label Sep 17, 2023
@drbenvincent drbenvincent linked an issue Sep 17, 2023 that may be closed by this pull request
@jpreszler
Copy link
Contributor Author

@drbenvincent The failure is coming from numpy when conftest is getting initialized, looks like it's the same issue on your website change pr. A new version was just released and the may have changed something. I've pinned version 1.24.4 (the most recent that github has before 1.26.0, I had 1.25.2 locally) and the tests are starting to run now.

@codecov
Copy link

codecov bot commented Sep 17, 2023

Codecov Report

Merging #240 (e2511f9) into main (234a0cd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #240   +/-   ##
=======================================
  Coverage   73.89%   73.89%           
=======================================
  Files          19       19           
  Lines        1149     1149           
=======================================
  Hits          849      849           
  Misses        300      300           
Files Changed Coverage Δ
causalpy/pymc_models.py 100.00% <ø> (ø)

@jpreszler
Copy link
Contributor Author

The issue is actually coming from https://github.com/pymc-devs/pytensor/blob/main/pytensor/link/c/cmodule.py#L2720 in pytensor. Since CausalyPy and PyTensor don't have upper limits on the numpy version, this line throws an exception since v1.26.0 (released on Sept. 16th) no longer has the get_info() method.

@drbenvincent
Copy link
Collaborator

drbenvincent commented Sep 18, 2023

Cool. I see you've dropped an issue in the pytensor repo pymc-devs/pytensor#441

What do you think the best way forward is? Something like this?

  • Temporarily pin numpy<1.26.0 like you've done and create an issue to resolve this in the future.
  • That future resolution would also presumably require a version of pytensor which resolves your issue BUG: using numpy code that has been deprecated in v1.26.0 pymc-devs/pytensor#441 ? EDIT: thinking about it, CausalPy should not really have strong opinions about what version of pytensor is being used... I think that direction could cause problems

@jpreszler
Copy link
Contributor Author

I think for anything that requires pytensor, pinning numpy<1.26.0 is really the only solution for now.

On the pytensor side, I suspect there's been a better way to access the blas options for some time and they just need to update the code and likely have a higher minimum numpy version, but I don't know enough about this to be sure. If this is the case, a new pytensor version with that fix would than provide a choice for downstream packages and moving to the new pytensor version is probably the best longer term route.

@drbenvincent drbenvincent merged commit 28d50b9 into pymc-labs:main Sep 18, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve math rendering of models in the docs
2 participants