stfc / fparser

This project maintains and develops a Fortran parser called fparser2 written purely in Python which supports Fortran 2003 and some Fortran 2008. A legacy parser fparser1 is also available but is not supported. The parsers were originally part of the f2py project by Pearu Peterson.
https://fparser.readthedocs.io
Other
64 stars 29 forks source link

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

Closed rupertford closed 1 year ago

rupertford commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/stfc/fparser/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stfc) | Coverage Δ | | |---|---|---| | [src/fparser/one/block\_statements.py](https://app.codecov.io/gh/stfc/fparser/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stfc#diff-c3JjL2ZwYXJzZXIvb25lL2Jsb2NrX3N0YXRlbWVudHMucHk=) | `79.09% <100.00%> (+0.56%)` | :arrow_up: | | [...parser/one/tests/test\_where\_construct\_stmt\_r745.py](https://app.codecov.io/gh/stfc/fparser/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stfc#diff-c3JjL2ZwYXJzZXIvb25lL3Rlc3RzL3Rlc3Rfd2hlcmVfY29uc3RydWN0X3N0bXRfcjc0NS5weQ==) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

arporter commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.