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

fparser1 where statement bug fix (closes #430) #431

Merged
merged 4 commits into from
Oct 3, 2023
Merged

Conversation

rupertford
Copy link
Collaborator

The fix is a one line change as we just need to return the original value of the string expression (using 'map'), rather than the string expression itself (F2PY_EXPR_TUPLE_1). For the example shown in issue #430:

subroutine columnwise_op_asm_m2v_lumped_inv_kernel_code()
  where (columnwise_matrix(:,:,cell) /= 0.0_r_solver)
     columnwise_matrix(:,:,cell) = 1.0_r_solver/columnwise_matrix(:,:,cell)
  end where
end subroutine columnwise_op_asm_m2v_lumped_inv_kernel_code

We used to get ...

!BEGINSOURCE
  SUBROUTINE columnwise_op_asm_m2v_lumped_inv_kernel_code()
    WHERE ( F2PY_EXPR_TUPLE_1 )
      columnwise_matrix(:,:,cell) = 1.0_r_solver/columnwise_matrix(:,:,cell)
    END WHERE
  END SUBROUTINE columnwise_op_asm_m2v_lumped_inv_kernel_code

But now get ....

!BEGINSOURCE
  SUBROUTINE columnwise_op_asm_m2v_lumped_inv_kernel_code()
    WHERE ( columnwise_matrix(:,:,cell) /= 0.0_r_solver )
      columnwise_matrix(:,:,cell) = 1.0_r_solver/columnwise_matrix(:,:,cell)
    END WHERE
  END SUBROUTINE columnwise_op_asm_m2v_lumped_inv_kernel_code

As we are no longer supporting fparser1, I propose that this change just goes on as is rather than adding any tests.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5018a64) 91.91% compared to head (0e6dc29) 91.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #431      +/-   ##
==========================================
+ Coverage   91.91%   91.96%   +0.04%     
==========================================
  Files          84       85       +1     
  Lines       13625    13644      +19     
==========================================
+ Hits        12524    12548      +24     
+ Misses       1101     1096       -5     
Files Coverage Δ
src/fparser/one/block_statements.py 79.09% <100.00%> (+0.56%) ⬆️
...parser/one/tests/test_where_construct_stmt_r745.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arporter
Copy link
Member

arporter commented Oct 2, 2023

As we are no longer supporting fparser1, I propose that this change just goes on as is rather than adding any tests.

I was tempted to agree with you but then realised that while it may be unsupported, it is actually critical for our LFRic functionality (as evidenced by the fact that we hit it) so I suggest adding one test to cover it.

@rupertford
Copy link
Collaborator Author

As we are no longer supporting fparser1, I propose that this change just goes on as is rather than adding any tests.

I was tempted to agree with you but then realised that while it may be unsupported, it is actually critical for our LFRic functionality (as evidenced by the fact that we hit it) so I suggest adding one test to cover it.

You and your reasonable logic. codecov fails as well so I should cover it.

@rupertford
Copy link
Collaborator Author

Ready for first review. Probably one for @arporter seeing as he expressed a preference for testing, but someone else can take it on as it is a simple change.

Copy link
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Change is good. New test added is pylint clean. CI is all green
No doc. update required.
Will proceed to merge.

@arporter arporter added ready for merge PR is waiting on final CI checks before being merged. and removed under review labels Oct 3, 2023
@arporter arporter merged commit f43a18f into master Oct 3, 2023
6 checks passed
@arporter arporter deleted the 430_where_bug branch October 3, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ready for merge PR is waiting on final CI checks before being merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants