8000 BUG: Make f2py work with intent(in out). by charris · Pull Request #5168 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

BUG: Make f2py work with intent(in out). #5168

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 10, 2014
Merged

BUG: Make f2py work with intent(in out). #5168

merged 2 commits into from
Oct 10, 2014

Conversation

charris
Copy link
Member
@charris charris commented Oct 9, 2014

Note that Fortran ignores spaces in this case, so that 'in out' is
treated as 'inout'.

Closes #479.

Rebased for backport to 1.9.0.

@charris
Copy link
Member Author
charris commented Oct 9, 2014

Strictly speaking, this fixes what could be regarded as abuse of Fortran, but as the Fortran compiler doesn't care, there is code in the wild that has this flaw.

@WarrenWeckesser
Copy link
Member

I don't think this simple fix is correct, for the reason you pointed out in #479.

Here's an example:

inout.f90

subroutine foo(x)
implicit none
real(4), intent(in out) :: x
dimension x(3)
x(1) = x(1) + x(2) + x(3)
return
end

subroutine bar(x)
implicit none
real(4), intent(inout) :: x
dimension x(3)
x(1) = x(1) + x(2) + x(3)
return
end

When the argument is not contiguous, foo doesn't complain (and fails to make the in-place change), while bar raises an exception:

In [5]: import inout

In [6]: x = np.arange(6, dtype=np.float32)[::2]

In [7]: x
Out[7]: array([ 0.,  2.,  4.], dtype=float32)

In [8]: inout.foo(x)

In [9]: x
Out[9]: array([ 0.,  2.,  4.], dtype=float32)

In [10]: inout.bar(x)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-10-6a9ef485b193> in <module>()
----> 1 inout.bar(x)

ValueError: failed to initialize intent(inout) array -- input not fortran contiguous

When the argument is contiguous, both versions work as expected:

In [12]: y = np.arange(3, dtype=np.float32)

In [13]: y
Out[13]: array([ 0.,  1.,  2.], dtype=float32)

In [14]: inout.foo(y)

In [15]: y
Out[15]: array([ 3.,  1.,  2.], dtype=float32)

In [16]: inout.bar(y)

In [17]: y
Out[17]: array([ 6.,  1.,  2.], dtype=float32)

@charris
Copy link
Member Author
charris commented Oct 9, 2014

I think there is a bug anyway, but from the fortran side intent(in out) is the same as intent(inout), so I'm not clear on what action you are proposing.

@WarrenWeckesser
Copy link
Member

Sorry, I'm not proposing an action. I'm just pointing out that the change in this PR doesn't fix the f2py bug. In the f2py generated wrapper of inout.f90, foo and bar should have exactly the same Python API, but they don't. foo is not treating its argument as intent(inout). I think the conversion of the string "in out" to "inout" has to occur elsewhere in crackfortran.py in order for them to be treated identically.

@WarrenWeckesser
Copy link
Member

Hold on, I think my previous comment applies to an outdated change (or maybe even a change not in this PR).

@charris
Copy link
Member Author
charris commented Oct 9, 2014

Yes, this PR should produce the same signature for both cases, the update just fixed the 'already present' check. Probably needs at least a test for compilation working...

@WarrenWeckesser
Copy link
Member

OK, this works for me now. When I made the above comment, I had used the fix mentioned in #479, and hadn't pulled the change from this PR. Sorry for the noise.

Yeah, a unit test for this would be good.

WarrenWeckesser
WarrenWeckesser reviewed Oct 9, 2014
View reviewed changes
numpy/f2py/crackfortran.py
vars[n]['intent'].append(c)
# Remove spaces so that 'in out' becomes 'inout'
tmp = c.replace(' ', '')
if not tmp in vars[n]['intent']:
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps tmp not in vars[...?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. I'm afraid to run pep8 on these files.

OK, I lied, running pep8 yields over a thousand PEP8 violations ;) I've fixed three.

@charris
Copy link
Member Author
charris commented Oct 10, 2014

@WarrenWeckesser I've used your example as a test.

@WarrenWeckesser
Copy link
Member

Looks good and works for me.

I don't know about the "Subproject commit" of doc/sphinxext. Is that intentional?

@charris
Copy link
Member Author
charris commented Oct 10, 2014

No, it's a d*mn artifact of the common rebase. I'm embarrassed I missed it, thanks for the catch.

charris added 2 commits October 9, 2014 19:39
@charris
BUG: Make f2py work with intent(in out).
1d6ef52 
Note that Fortran ignores spaces in this case, so that 'in out' is
treated as 'inout'.

Closes numpy#479.
@charris
TST: Add basic test for compiling with intent(in out).
e98686d 
This checks that the compilation works and that the expected error
is raised when non-contiguous arrays are passed as intent(in out).
charris added a commit that referenced this pull request Oct 10, 2014
@charris
Merge pull request #5168 from charris/f2py-space-fix
6457686 
BUG: Make f2py work with intent(in out).
@charris charris merged commit 6457686 into numpy:master Oct 10, 2014
@charris charris deleted the f2py-space-fix branch October 10, 2014 16:18
@charris
Copy link
Member Author
charris commented Oct 10, 2014

@juliantaylor You might want to backport this.

juliantaylor added a commit that referenced this pull request Oct 10, 2014
@juliantaylor
Merge pull request #5168 from charris/f2py-space-fix
e2851d2 
BUG: Make f2py work with intent(in out).
@juliantaylor
Copy link
Contributor

pushed to the branch, thanks
note you can also create a new PR from these rebased branches if you prefer, just create a second PR from the same branch using a different merge target, easiest done by simply editing the pull request creation url

@charris
Copy link
Member Author
charris commented Oct 11, 2014

@pearu Pearu, what is the status of fparser?

@pearu
Copy link
Contributor
pearu commented Oct 12, 2014

Sorry, guys, but it seems too me that some comments bark up the wrong tree.

First, Fortran 77 and newer Fortran standards in fixed mode completely ignore spaces. In free mode, however, spaces are treated as in any popular programming language. For example,
i n t e n t (i n o u t) is valid in fixed mode, but not in free mode. Now, intent(in out) case is special. gfortran accepts it in free mode. However, Fortran 2003 standard (2008 document was not available at the moment) does not accept it in free mode, only intent(inout) would be valid. So, I guess, intent(in out) is just a compiler vendor initiative. Also, it would be easy to patch crackfortran.py to allow intent(in out). It would be overkill to implement full Fortran parser just because of this single issue.

Second, f2py "parser" (crackfortran.py) is not a conventional parser as it uses regular expressions. It was a practical choice at the time of creating f2py when the main aim was to have a robust extension source generator, parsing Fortran had a secondary priority. When f2py fails to parse, one can always relay on writing signature files by hand.

Implementing Fortran parser with full standard support is far from trivial, fparser project (it exists within f2py google code project) has been created just for that. Since developing f2py financial support had stopped some time ago, I haven't had resources to finish fparser. So, fparser status is 'in hold'.

@charris
Copy link
Member Author
charris commented Oct 12, 2014

@pearu Lahey seems to use the construction in their documentation. We have already patched crackfortran.py to deal with the problem.

I was asking about fparser in the hope that there might be a newer version of f2py that would be easier to maintain, but didn't have much hope for that. Parsing Fortran, with all it's versions and idiosyncrasies is not a trivial problem and f2py works wonders, but it is not easy to maintain.

@njsmith njsmith mentioned this pull request Sep 24, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

f2py fails creating an interface file
4 participants
@charris @WarrenWeckesser @juliantaylor @pearu
0