Transformation utility to fix sequence association #173
Conversation
…-syntax-for-array-arguments
…-syntax-for-array-arguments
Codecov Report
@@ Coverage Diff @@
## main #173 +/- ##
==========================================
+ Coverage 92.15% 92.17% +0.02%
==========================================
Files 92 93 +1
Lines 16765 16804 +39
==========================================
+ Hits 15449 15489 +40
+ Misses 1316 1315 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
What's the appropriate way to deal with derived type objects. I treat them as |
|
Hi @rolfhm , first of all thanks for looking into this; having a utility mechanism for resolving this type of argument-passing style is certainly very useful and powerful. As for the implementation style, I actually think this is already looking pretty good. I personally would prefer all utility routines to have at least small docstrings (I know! 😉), and I haven't done a thorough code review yet, but for a first implementation this is looking quite ok to me. Yes, eventually we might want to add a The one key thing to point out is that you will need some testing for this, as we do not allow individual PRs to degrade test coverage beyond a small threshold. But this can be done with a procedural interface, pretty much as we do for the other utility transformations (try to test it standalone first, not as part of SCC). The final point I noted is that, if you integrate this straight into the SCC transformation, which totally makes sense, it might be good to have a flag to switch this off (a constructor flag to the SCCBaseTransformation, just in case). This can be on by default, but having an off-switch can be useful, just in case things go wrong. Other than that, this looks like it's well on its way, so please feel free to put this into proper PR review mode once you've added a test or two. 😄 |
|
Spent too much time getting into the details on how to simplify expressions, but I resisted the temptation to try to resolve all the nested Sums. The If your doing arithmetic operations on the Loki versions of |
There was a problem hiding this comment.
First of all, apologies for the delay in getting to this, and many thanks for this contribution!
I agree with Michael, the implementation looks very good and putting this in as a Loki transformation helper utility does sound look the right place to me.
My main request would be to not call this "scalar syntax". The correct term in Fortran is "sequence association" and I'd vote that this should be used throughout here (i.e., command-line argument, CMake options and Python code).
The testing is great. Note that you should be able to just compare to strings rather than instances of the relevant expression nodes, which would make the test a bit shorter and more accessible - but that's a minor detail.
The product_value utility is currently under-tested (see test coverage). Would it be possible to add 2-3 more cases to the test which trigger the remaining cases?
reuterbal
changed the title
…-syntax-for-array-arguments
…-syntax-for-array-arguments
|
Agree with the name change. I changed the option to |
| call sub_x(array(2, 2), 2) | ||
| call sub_x(array(m, 1), k) | ||
| call sub_x(array(m-1, 1), k-1) | ||
| call sub_x(array(a%b%c, 1), a%b%d) |
There was a problem hiding this comment.
Can we add here something like
call sub_x(array(m+0, 1), k)
call sub_x(array(2*m-m, 1), k)
to cover the other cases in product_value?
There was a problem hiding this comment.
I was looking into it, but discovered that the code crashes if the dimension inside the routine is given by a Sum or a Product, for example dimension(k+1) and dimension(-k), respectively.
The solution is to modify the process_symbol routine to recursively process such cases, but I'm wondering if I've been really overthinking the problem. Since sequence association is always contiguous, I think we could just plug in the relevant array dimensions from the caller routine. At least the two calls in this example case produce the same output:
program crashdummies
use blarg, only: blurg
implicit none
real, dimension(10,5) :: A
integer :: j
A = 0.
call blurg(A(5:10,1))
do j = 1,3
print *, A(:,j)
enddo
print *
A = 0.
call blurg(A(5:19,1))
do j = 1,3
print *, A(:,j)
enddo
end program crashdummies
module blarg
contains
subroutine blurg(x)
implicit none
real, dimension(15) :: x
integer :: i
do i = 1, 15
x(i) = 1.
enddo
end subroutine blurg
end module blarg
Would the inliner have any problem with this?
There was a problem hiding this comment.
Fantastic, simplicity is always better and this looks really neat now! Pylint has flagged a few redundant symbols and imports now.
I don't think the inliner should have a problem with this, at least there is a test that includes ranges:
loki/tests/test_transform_inline.py
Line 374 in a2f2428
|
And now it passes the code checks again :) |
…-syntax-for-array-arguments
There was a problem hiding this comment.
First of all, many thanks for a very comprehensive piece of work! After the clean-up this all looks very well implemented and tested, so all good from my side. Very much appreciate this one, as it's a very useful utility, even outside of the primary target context! 🙏
Since this the commit history for this one has a few twists and turns, I'd propose doing a squash merge, but other than that, GTG from my side.
|
Should I do a squash commit manually, or is it possible to do it as part of the merge? |
|
I'll take care of that, just waiting for the test suite to finish after the last PR merge |
Includes the routine
fix_scalar_syntaxthat transforms actual arguments like a(1,1) passed to array dummy arguments into array syntax. It needs some documentation and a test for derived types, so making a draft pull request.Is the format OK? Should there be some kind of transformation class? Have I put it in the right directory?