Skip to content

BUG,ENH: Fix negative bounds for F2PY #21256

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

Merged
merged 5 commits into from
Mar 31, 2022
Merged

Conversation

HaoZeke
Copy link
Member

@HaoZeke HaoZeke commented Mar 27, 2022

Closes #20853.

Some alternatives were suggested in the issue, namely disabling all checks, which is still an option but seems sub-optimal.

However, there are some implementation notes which are important.

This PR changes the behaviour of the python caller, which is now responsible for being the right size. That is:

subroutine foo(is_, ie_, arr, tout)
 implicit none
 integer :: is_,ie_
 real, intent(in) :: arr(is_:ie_)
 real, intent(out) :: tout(is_:ie_)
 tout = arr
end

After compiling f2py -m blah blah.f90, this can be called with:

import numpy as np
import blah
xlow = -6
xhigh = 4
xvec = np.arange(12)
def ubound(xl, xh):
  return abs(xl) + abs(xh) + 1
rval = blah.foo(is_ = xlow, ie_ xhigh,
         arr = xvec[:ubound(xlow, xhigh))
expval = np.arange(11, dtype = np.float32)
np.allclose(rval, expval) 

Essentially, the caller is now responsible in this scenario for ensuring that the size of the array passed in is correct, and this means for the most part that the slice should be calculated as above.

@HaoZeke HaoZeke requested a review from pearu March 27, 2022 17:52
@HaoZeke HaoZeke added this to the 1.22.4 release milestone Mar 27, 2022
@HaoZeke HaoZeke requested a review from melissawm March 27, 2022 17:54
@HaoZeke HaoZeke changed the title BUG: Fix negative bounds for F2PY BUG,ENH: Fix negative bounds for F2PY Mar 27, 2022
@charris
Copy link
Member

charris commented Mar 27, 2022

Does this work with SciPy?

@HaoZeke
Copy link
Member Author

HaoZeke commented Mar 27, 2022

Does this work with SciPy?

Yup, in so far as there are no examples of the original issue showing up in SciPy and the change in behavior is only triggered for the case when:

  • The dimensions of an array are defined by other input variables
    • Essentially for an array of size xlow:xhigh which are also variables

I also ran runtests.py cleanly (on scipy master). @melissawm would be the best person to confirm that SciPy works though :)

@HaoZeke HaoZeke marked this pull request as draft March 29, 2022 11:10
@HaoZeke HaoZeke marked this pull request as ready for review March 29, 2022 14:12
Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

Thanks, @HaoZeke for this!

I have a suggestion for detecting the cyclic dependence using coeffs_and_deps data only that targets the original issue more precisly.

Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
@HaoZeke HaoZeke requested a review from pearu March 29, 2022 18:17
Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

I have a few minor nits but otherwise looks good!

Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
@HaoZeke HaoZeke requested a review from pearu March 29, 2022 20:27
Copy link
Contributor

@pearu pearu left a comment

Choose a reason for hiding this comment

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

LGTM!

@melissawm
Copy link
Member

Let's put this in - thanks @HaoZeke !

@melissawm melissawm merged commit 04bf405 into numpy:main Mar 31, 2022
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Mar 31, 2022
charris pushed a commit to charris/numpy that referenced this pull request Mar 31, 2022
Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Mar 31, 2022
@charris charris removed this from the 1.22.4 release milestone Mar 31, 2022
melissawm pushed a commit to melissawm/numpy that referenced this pull request Apr 12, 2022
Co-authored-by: Pearu Peterson <pearu.peterson@gmail.com>
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.

BUG: [Regression 1.21.4 -> 1.22.0] f2py cannot handle negative bounds as arguments anymore
4 participants