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

Unpin scipy upper version #972

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Unpin scipy upper version #972

merged 3 commits into from
Aug 15, 2024

Conversation

ferrine
Copy link
Member

@ferrine ferrine commented Aug 13, 2024

Description

Related Issue

Checklist

Type of change

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

@ferrine
Copy link
Member Author

ferrine commented Aug 13, 2024

besides expected errors there is one that comes from mypy

!!!!!!!!!
2 files unexpectedly failed.
/home/runner/work/pytensor/pytensor/pytensor/gradient.py
/home/runner/work/pytensor/pytensor/pytensor/graph/basic.py
These files did not fail before, so please check the above output for errors in {PosixPath('/home/runner/work/pytensor/pytensor/pytensor/graph/basic.py'), PosixPath('/home/runner/work/pytensor/pytensor/pytensor/gradient.py')} and fix them.
You can run `python scripts/run_mypy.py --verbose` to reproduce this test locally.

@ferrine
Copy link
Member Author

ferrine commented Aug 13, 2024

I also think of simplifying this part of code, it has a lot of redundant logic that is already managed by simple np.asarray

diff --git a/pytensor/tensor/elemwise.py b/pytensor/tensor/elemwise.py
index de966f1a7..a0892dfd0 100644
--- a/pytensor/tensor/elemwise.py
+++ b/pytensor/tensor/elemwise.py
@@ -767,31 +767,17 @@ class Elemwise(OpenMPOp):
         for i, (variable, storage, nout) in enumerate(
             zip(variables, output_storage, node.outputs)
         ):
-            if getattr(variable, "dtype", "") == "object":
-                # Since numpy 1.6, function created with numpy.frompyfunc
-                # always return an ndarray with dtype object
-                variable = np.asarray(variable, dtype=nout.dtype)
+
+            variable = np.asarray(variable, dtype=nout.dtype)

             if i in self.inplace_pattern:
                 odat = inputs[self.inplace_pattern[i]]
                 odat[...] = variable
                 storage[0] = odat

-            # Sometimes NumPy return a Python type.
-            # Some PyTensor op return a different dtype like floor, ceil,
-            # trunc, eq, ...
-            elif not isinstance(variable, np.ndarray) or variable.dtype != nout.dtype:
-                variable = np.asarray(variable, nout.dtype)
-                # The next line is needed for numpy 1.9. Otherwise
-                # there are tests that fail in DebugMode.
-                # Normally we would call pytensor.misc._asarray, but it
-                # is faster to inline the code. We know that the dtype
-                # are the same string, just different typenum.
-                if np.dtype(nout.dtype).num != variable.dtype.num:
-                    variable = variable.view(dtype=nout.dtype)
-                storage[0] = variable
+            storage[0] = variable
             # numpy.real return a view!
-            elif not variable.flags.owndata:
+            if not variable.flags.owndata:
                 storage[0] = variable.copy()
-            else:
-                storage[0] = variable

@ferrine ferrine mentioned this pull request Aug 13, 2024
@ferrine
Copy link
Member Author

ferrine commented Aug 13, 2024

@ricardoV94 how are these tests are supposed to pass?

def test_inner_composite(mode):

Why did they pass before?

@ferrine
Copy link
Member Author

ferrine commented Aug 14, 2024

Wow, looking into the broken test closer, it should have been a failing test!

before the PR

image

after the change the dtype of out is float16 as declared in the graph and we see overflow

@ricardoV94 ricardoV94 changed the title Unpin scipy Unpin scipy upper version Aug 14, 2024
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 81.69%. Comparing base (29183c7) to head (d82b1e5).
Report is 101 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/scalar/basic.py 75.00% 3 Missing ⚠️
pytensor/gradient.py 60.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #972   +/-   ##
=======================================
  Coverage   81.69%   81.69%           
=======================================
  Files         182      182           
  Lines       47585    47591    +6     
  Branches    11584    11586    +2     
=======================================
+ Hits        38875    38880    +5     
  Misses       6518     6518           
- Partials     2192     2193    +1     
Files with missing lines Coverage Δ
pytensor/graph/basic.py 88.66% <100.00%> (ø)
pytensor/tensor/elemwise.py 88.54% <100.00%> (+0.16%) ⬆️
pytensor/gradient.py 77.49% <60.00%> (-0.15%) ⬇️
pytensor/scalar/basic.py 80.41% <75.00%> (+0.03%) ⬆️

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.

Thanks @ferrine

@ricardoV94
Copy link
Member

What's with mypy? I guess it's not related, but can you fix it?

@ricardoV94
Copy link
Member

@ferrine can you clean the commits into just 2:

  1. scipy stuff
  2. mypy stuff
    ?

@ferrine
Copy link
Member Author

ferrine commented Aug 14, 2024

Yes, will do soon

fix: cast to elemwise outputs to their respective dtypes

fix: Relax scipy dependency, should work in both cases

style: black

wrap with asarray

fix: make elemwise test check against dtype in the graph

fix scalar issues

Update pytensor/scalar/basic.py

Co-authored-by: Ricardo Vieira <[email protected]>

fix test

add a clarifying comment to checking nan

fix: bool is deprecated in numpy

deps: bound scipy version

improve test
@ferrine ferrine merged commit a3f0a4e into main Aug 15, 2024
59 of 60 checks passed
@ferrine ferrine deleted the unpin-scipy branch August 15, 2024 08:48
@ricardoV94 ricardoV94 mentioned this pull request Sep 15, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants