BUG: Adds support for array parameter declaration in fortran code #15457

melissawm · 2020-01-28T18:26:15Z

Hello,

This PR is supposed to add support for array parameter declaration in fortran code. Previously, this resulted in an error when trying to use array parameter values to define other variables in the same code (see bug #11612)

I also added some tests, including one which is supposed to fail right now because this fix does not include the special case of array parameters with 0-based indexing in fortran.

Closes #11612
Closes #8730

numpy/f2py/crackfortran.py

pearu

Thanks for working on this! I agree that the PR could be split into several smaller ones each fixing one issue at the time. Otherwise, PR-s may grow with no limit.

numpy/f2py/crackfortran.py

numpy/f2py/tests/test_parameter.py

numpy/f2py/crackfortran.py

melissawm · 2020-03-04T14:11:45Z

Hi all, I just added a few comments to let you know that I'm planning on working on more delicate issues in separate PR's so we can do proper testing and review. The code above works for simple cases. I don't know if this process is the recommended one, let me know what you think. Thanks!

mattip · 2020-03-05T11:38:13Z

Just to be clear: you want to merge this as-is with the problematic indexing? If so, could you add an TODO: fix ... in the code?

eric-wieser · 2020-03-05T12:09:53Z

My understanding was that #15706 was extracted from this, so should be merged first.

Edit: Looks like they were made orthogonal, so the order matters less

numpy/f2py/crackfortran.py

melissawm · 2020-04-21T21:22:27Z

Sorry, this was a bit messy but I think I have a better version now. I also added two tests, one which is meant to record the fact that for now we can't parse multidimensional parameter arrays, since this would imply executing fortran code before the extension module is built (e.g. the intrinsic reshape function).

The code changed quite a bit because I tried incorporating all suggestions while also improving my own understanding of how crackfortran works. Thanks for reviewing and for the patience in waiting for the fix!

melissawm · 2020-06-25T17:35:53Z

@pearu can you take a look when you have the time? Thanks!

numpy/f2py/capi_maps.py

numpy/f2py/crackfortran.py

eric-wieser · 2020-06-25T18:17:31Z

numpy/f2py/crackfortran.py

+        # This is an array parameter.
+        # First, we parse the dimension information
+        a = [item[9:] for item in varsn['attrspec'] if item[:9] == 'dimension'][0]
+        dimrange = a.lstrip('(').rstrip(')')


probably safer as

Suggested change

dimrange = a.lstrip('(').rstrip(')')

a = a.strip() # does fortran care about whitespace?

if a[0] != "(" or a[-1] != ")":

raise ValueError(...)

dimrange = a[1:-1]

Otherwise you could strip off unbalanced parentheses

numpy/f2py/crackfortran.py

numpy/f2py/tests/src/parameter/constant_array.f90

eric-wieser · 2020-06-25T18:51:26Z

numpy/f2py/crackfortran.py

+    #    is often incompatible with the original fortran indexing)
+    # 2) allow the parameter array to be accessed in python as a dictionary with fortran
+    #    indices as keys
+    # We are choosing 2 for now.


One option here would be:

class FortranParamArray: def __init__(self, indices, values): self._indices = indices self._values = values def __getitem__(self, index): return self._values[self._indices.index(index)]

And pass the range object in as the indices argument.

I didn't consider this because the f2py code does not use OOP and I think this might be a part of a larger refactor in the future.

pearu

Thanks, @melissawm for developing param_eval!
My only concern is that it is too rigorous in reporting unsupported (read: notimplemented) features as failures while f2py may not need these features when creating the wrapper code.

The used naming convention in "crackfortran.py" has an actual purpose: be forgiving on things that do not matter :)

pearu · 2020-06-25T20:44:02Z

numpy/f2py/tests/test_parameter.py

+        y = self.module.foo_array_any_index(x)
+        assert_equal(y, x.reshape((2,3), order='F'))
+
+    def test_constant_array_multidimensional(self):


Why this test is expected to fail? f2py should pick up only the following information from the code:

subroutine foo_multidimensional_array(x) integer, parameter:: dp = selected_real_kind(15) real(dp), intent(inout):: x(5, 3, 2) end subroutine

The parameter y is irrelevant here and all the issues with it should be ignored by f2py.

pearu · 2020-06-25T20:49:09Z

numpy/f2py/crackfortran.py

@@ -2873,6 +2906,84 @@ def analyzevars(block):
 analyzeargs_re_1 = re.compile(r'\A[a-z]+[\w$]*\Z', re.I)


+def param_eval(n, varsn, v, g_params, params):


param_eval is a complicated function and deserves explicit testing.

Let me know if the tests I added are enough.

pearu · 2020-06-25T20:55:32Z

numpy/f2py/crackfortran.py

                        d = str(params[d])
+                    if d[:d.find("(")] in params:


nit1: a white space before ( in d would break this code.
nit2: consider

>>> d='abc' >>> d[:d.find('(')] 'ab'

Hence, d[:d.find("(")] in params is buggy. Suggest checking .find(..) return value against -1 before continuing.

pearu · 2020-06-25T20:59:12Z

numpy/f2py/crackfortran.py

-            except Exception as msg:
-                params[n] = v
-                outmess('get_parameters: got "%s" on %s\n' % (msg, repr(v)))
+            params[n] = param_eval(n, vars[n], v, g_params, params)


Since param_eval does not implement the corresponding Fortran specification in full, and often it does not matter (see test_constant_array_multidimensional), the try-except block should remain.

pearu · 2020-06-25T21:07:35Z

numpy/f2py/crackfortran.py

+                dimrange = range(int(dimrange[0]), int(dimrange[1])+1)
+        else:
+            print(n)
+            raise Exception('param_eval: multidimensional array parameters not supported: %s\n' % repr(n))


I would just report this via outmess rather than raising an exception because the corresponding case is often irrelevant for the f2py task.

melissawm · 2020-07-20T22:26:32Z

I think I addressed everything, feedback is appreciated. Thanks once again for the patience and for the time spent reviewing this!

eric-wieser · 2020-07-20T22:28:15Z

numpy/f2py/crackfortran.py

+                    ' supported: %s\n' % repr(n))
+
+        # Parse parameter value
+        v = v.lstrip('(/').rstrip('/)').split(',')


strip is likely not suitable here, it maps (/(a), b/) to a), b.

I updated the code to address this, but I'm not sure if another approach would be preferable.

v = (v[2:-2] if v.startswith('(/') else v).split(',')

eric-wieser · 2020-08-14T21:34:26Z

numpy/f2py/crackfortran.py

+
+      then `d = 2` and we return immediately, with
+
+      `param_parse(d, params) = '2'`


Can you write this in the form of a doctest, with >>>?

Let me know if this is what you had in mind. I could write a proper doctest but that would involve populating the entire param dictionary which I don't think is the idea. I tried looking for similar tests in the rest of the code but couldn't find any. Thanks again for your patience on this, I'm still learning! Feel free to keep commenting if you think anything can be improved.

I've put a suggestion above - but yes, a minimally populated params dictionary is exactly what I'm looking for, since otherwise it's hard to tell what form that argument takes.

numpy/f2py/crackfortran.py

melissawm · 2021-10-04T14:04:04Z

(Sorry for the mess in the history - I didn't squash yet because it might make it harder to review. I'll do that once this is ready to merge)

pearu

Found one bug and have a few nits. Otherwise, looks good!

numpy/f2py/crackfortran.py

HaoZeke · 2021-12-15T00:23:32Z

@melissawm could you squash this before I rebase it?

melissawm · 2021-12-15T13:37:43Z

Done, thanks @HaoZeke

HaoZeke · 2021-12-19T10:01:03Z

Thanks @melissawm. This actually rebases cleanly, the relevant branch is here. So:

# Grab the F2PY refactor PR
gh pr checkout 20481
# Add it to your fork, assumes origin to be the fork
git push origin HEAD:f2pyRefactor
# Goto your branch for this PR
git checkout f2py-param-fix
# Rebase onto the refactor
git rebase -i f2pyRefactor
# Push the updated branch
git push -f

In case it does not apply cleanly, the other option is to checkout the branch mentioned above and force push that to this PR. Let me know if this can be made clearer for the other PRs :)

HaoZeke

Perhaps a release note for param_eval would be nice, otherwise this is ready.

charris · 2022-06-09T16:40:29Z

@melissawm Needs rebase. Seems this was about ready to go.

HaoZeke

This needs some work after the changes introduced in the recent PRs, but the issue still exists.

bilderbuchi · 2022-09-26T11:42:22Z

@melissawm what's the status of this PR? It seems it was ~finished/ready to go already?
I just hit #11612, and this PR got my hopes for a fix up :D

charris · 2022-11-15T18:05:07Z

@melissawm Is this still in progress?

@pearu @HaoZeke Have your requested changes been dealt with?

charris · 2022-11-15T18:05:19Z

Needs rebase.

HaoZeke

This works, and fixes the still present #11612 so it would be great to have this in :)

I think all the requested changes were made, and subsequent iterations can clean up any new issues.

mattip · 2023-04-20T19:14:10Z

@pearu could you take a look?

HaoZeke · 2023-11-20T00:17:17Z

Would be nice to have this in :) I think any additional changes can be done in followups if necessary.

HaoZeke · 2023-11-22T21:03:15Z

@pearu could you take a look / remove the review changes requested note? Can't be merged otherwise

EDIT: Dismissing the review to get this in :)

Stale review, changes are good to go.

melissawm mentioned this pull request Jan 28, 2020

f2py cannot handle array PARAMETER #11612

Closed

eric-wieser reviewed Jan 28, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

pearu requested changes Jan 28, 2020

View reviewed changes

charris added 00 - Bug component: numpy.f2py labels Jan 28, 2020

eric-wieser reviewed Feb 8, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

eric-wieser reviewed Feb 8, 2020

View reviewed changes

numpy/f2py/tests/test_parameter.py Outdated Show resolved Hide resolved

eric-wieser reviewed Mar 3, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

eric-wieser reviewed Mar 5, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

seberg mentioned this pull request Mar 6, 2020

MAINT: Fixing typos in f2py comments and code. #15706

Merged

melissawm force-pushed the f2py-param-fix branch from 5d0f802 to 9356efc Compare April 21, 2020 21:18

eric-wieser reviewed Jun 25, 2020

View reviewed changes

numpy/f2py/capi_maps.py Outdated Show resolved Hide resolved

eric-wieser reviewed Jun 25, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

eric-wieser reviewed Jun 25, 2020

View reviewed changes

numpy/f2py/crackfortran.py Show resolved Hide resolved

eric-wieser reviewed Jun 25, 2020

View reviewed changes

numpy/f2py/tests/src/parameter/constant_array.f90 Outdated Show resolved Hide resolved

eric-wieser reviewed Jun 25, 2020

View reviewed changes

pearu requested changes Jun 25, 2020

View reviewed changes

melissawm marked this pull request as draft June 30, 2020 17:35

melissawm marked this pull request as ready for review July 20, 2020 22:22

eric-wieser reviewed Jul 20, 2020

View reviewed changes

eric-wieser reviewed Aug 14, 2020

View reviewed changes

numpy/f2py/crackfortran.py Outdated Show resolved Hide resolved

melissawm force-pushed the f2py-param-fix branch from 080f874 to fe0d634 Compare September 4, 2020 16:00

pearu previously requested changes Oct 25, 2021

View reviewed changes

melissawm force-pushed the f2py-param-fix branch from d953599 to 68e308e Compare October 25, 2021 14:53

melissawm force-pushed the f2py-param-fix branch from 68e308e to 9e792eb Compare December 15, 2021 13:37

HaoZeke approved these changes Apr 16, 2022

View reviewed changes

HaoZeke requested changes Jun 9, 2022

View reviewed changes

charris added the 09 - Backport-Candidate PRs tagged should be backported label Sep 26, 2022

charris removed the 09 - Backport-Candidate PRs tagged should be backported label Dec 6, 2022

HaoZeke force-pushed the f2py-param-fix branch from 9e792eb to 5512958 Compare April 15, 2023 19:45

HaoZeke approved these changes Apr 15, 2023

View reviewed changes

HaoZeke requested a review from pearu April 15, 2023 19:46

HaoZeke force-pushed the f2py-param-fix branch from 5512958 to 0965749 Compare November 20, 2023 00:15

HaoZeke requested a review from mattip November 20, 2023 00:16

melissawm and others added 2 commits November 22, 2023 18:56

BUG, ENH: Adds support for parameter arrays in fortran code

01a7265

TST: Cleanup rebase issue, no prints in tests

f2b3638

HaoZeke force-pushed the f2py-param-fix branch from 0965749 to f2b3638 Compare November 22, 2023 18:56

rgommers added this to the 2.0.0 release milestone Nov 23, 2023

HaoZeke merged commit a820164 into numpy:main Nov 23, 2023
60 checks passed

f2py core automation moved this from In progress to Done Nov 23, 2023

-        dimrange = a.lstrip('(').rstrip(')')
+        a = a.strip()  # does fortran care about whitespace?
+        if a[0] != "(" or a[-1] != ")":
+            raise ValueError(...)
+        dimrange = a[1:-1]

		@@ -2873,6 +2906,84 @@ def analyzevars(block):
		analyzeargs_re_1 = re.compile(r'\A[a-z]+[\w$]*\Z', re.I)


		def param_eval(n, varsn, v, g_params, params):


		then `d = 2` and we return immediately, with

		`param_parse(d, params) = '2'`

BUG: Adds support for array parameter declaration in fortran code #15457

BUG: Adds support for array parameter declaration in fortran code #15457

Conversation

melissawm commented Jan 28, 2020 • edited Loading

pearu left a comment

Choose a reason for hiding this comment

melissawm commented Mar 4, 2020

mattip commented Mar 5, 2020

eric-wieser commented Mar 5, 2020 • edited Loading

melissawm commented Apr 21, 2020

melissawm commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pearu Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissawm commented Jul 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melissawm commented Oct 4, 2021

pearu left a comment

Choose a reason for hiding this comment

HaoZeke commented Dec 15, 2021

melissawm commented Dec 15, 2021

HaoZeke commented Dec 19, 2021

HaoZeke left a comment

Choose a reason for hiding this comment

charris commented Jun 9, 2022

HaoZeke left a comment

Choose a reason for hiding this comment

bilderbuchi commented Sep 26, 2022

charris commented Nov 15, 2022

charris commented Nov 15, 2022

HaoZeke left a comment • edited Loading

Choose a reason for hiding this comment

mattip commented Apr 20, 2023

HaoZeke commented Nov 20, 2023

HaoZeke commented Nov 22, 2023 • edited Loading

melissawm commented Jan 28, 2020 •

edited

Loading

eric-wieser commented Mar 5, 2020 •

edited

Loading

pearu left a comment •

edited

Loading

pearu Jun 25, 2020 •

edited

Loading

HaoZeke left a comment •

edited

Loading

HaoZeke commented Nov 22, 2023 •

edited

Loading