Closed issue cleanup

[haanahfrost]

WIP draft (follow up to #3506) - some references have been resolved but I have left many for review instead of guessing.
I found that a big portion of the references were closed as 'completed' via a PR only 'towards' it or a duplicate which was also closed, but the specific comment guards were never migrated to use whatever the issue delivered.

Removed via deletion:

#1298, #3143, #2004
#1483/#1392/#298 chain (extract_node.py),
#257 (contradicted by fix in #2707), #1387
#685 (fparser2.py — default loop increment now uses the loop start
node's datatype, approved with Andy)
#730 (examples/gocean/eg5/profile/README.md)
#824, #1991, #2049 (doc/developer_guide/psy_data.rst)
#2125 (inline_trans_test.py)
#813 (lib/read_only/lfric/Makefile)

Removed pointer but kept limitation comment:

SIR backend cluster: #468, #521, #741, #470 - all closed "won't fix"
since SIR is no longer actively developed
#2448 (closed but kept comment)
#1854 (sir_trans_loop.py - reworded to state current fact)

Removed via repointing to an open issue:

• transformations do not recognise typos in option names #613 → Transformation options refactoring #2668 (both loop_tiling_2d_trans.py and profiling.rst)
• Re-implement f2pygen using fparser2 #240 → Extend test compilation support to include Algorithm layer #284 (lfric_alg_test.py)
• Preprocessor directives causing syntax error #3038 → Investigate use of TreeSitter #3083 (4 sites in the treesitter frontend)

Removed and tested behavioural changes:

• #1537 (assignment_trans.py) - removed the array-range SymbolicMaths
  workaround
• #1113 (modify.py example) - specialise() now accepts **kwargs;
  replaced the manual symbol.datatype = ... line with passing it directly.
• #2592 (fortran.py) - swapped to node.symbol, then removed the now-
  redundant container. Backend tests pass
• #229 and #2856 - fixed order and removed the unneeded try/except, updated README to describe current behaviour

#3178 — resolved independently by @LonelyCat124 in #3518 (unrelated work
that happened to touch the same TODO), not by this PR.

Flagged - needs review

Closed but code still visibly does outdated thing/workaround:

• #1010 - 7 sites (backends_transform.py ×2, lfric_loop_test.py ×2, kern_call_arg_list.py ×2, lfric_loop.py). Each site still exhibits old behaviour

• #658 - sir.py, assignment_test.py, type_convert_intrinsic_test.py, intrinsic_call.py. Type-determination capability still missing; one site is a live xfail.

• #3135 - datatypes.py. Closed via #3436

• #1351 - kern_call_arg_list.py. Stub generator still uses .remove("dof")/.remove("owned_dof").

• #1631

• #1806 - lfric_alg.py ×2, tl2ad.py, lfric_adjoint_harness.py. All still use fparser1.

• #1930 - kern_call_arg_list.py docstring. String-format migration not done.

• #1570 - lfric.py. Closed via #3382

• #2623 - 3 test sites. commented-out assert LFRicBuild(tmpdir).code_compiles(psy)

• #1772 / #1453 -examples/psyad/eg2/Makefile. Manual sed rewrite still present.

• #365 - nemo.py, valid_kernel.

• #309 — kernels_trans.py. Traced to #399→#485, but DependencyTools never adopted here.

• #2236 -test_lfric_adjoint.py, lfric_adjoint.py. Successor #2347 also closed; workaround still present.

• #2596 - adjoint_visitor.py. Points at a merged PR; author unsure if related to #1946 or #2729.

• #1556 - test_lfric_adjoint.py

• #753 - ~8 sites across doc/developer_guide/psykal.rst, lfric.py, tests.

• #1272 - goloop_test.py.

• #1504 -fparser2.py. Reverted from an earlier premature deletion flagging rather than removing.

• #1807 - lfric_adjoint.py.

• #2823 - call_test.py. Reverted from an earlier premature deletion flagging rather than removing.

• #3157 - maximal_omp_parallel_region_trans.py

• #3205 - maximal_omp_parallel_region_trans.py.

• #3265 - reference2arrayrange_trans_test.py. Reverted from an earlier premature deletion flagging rather than removing.

• #1235 - 7 xfail sites in psyad tests.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d19716d) to head (bd138ad).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##            master     #3535   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          397       397           
  Lines        55629     55626    -3     
=========================================
- Hits         55629     55626    -3     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sergisiso]

#1010 - 7 sites (backends_transform.py ×2, lfric_loop_test.py ×2, kern_call_arg_list.py ×2, lfric_loop.py). Each site still exhibits old behaviour

Replace with #2905

#658 - sir.py, assignment_test.py, type_convert_intrinsic_test.py, intrinsic_call.py. Type-determination capability still missing; one site is a live xfail.

@LonelyCat124 This has to do with return type of intrinsics. Can you have a look but my impression is that we need a new issue.

#3135 - datatypes.py. Closed via #3436

Replace with #3538

#1631

Remove TODOs and associated comments

#1806 - lfric_alg.py ×2, tl2ad.py, lfric_adjoint_harness.py. All still use fparser1.

Replace with #2151

#1570 - lfric.py. Closed via #3382

Remove TODO and associated comment

#2623 - 3 test sites. commented-out assert LFRicBuild(tmpdir).code_compiles(psy)

Replace with #3540

Good job @haanahfrost . See my comments inline and above for some of the flagged TODOs (I will continue in another message). Also, this may not be just a one-off script, we could still close issues manually which wouldn't trigger the CI check. So we may want to run this from time to time. Could you add your script in the utils folder?

[sergisiso]

Add something like:

.. warning:: SIR is no longer actively developed, psyclone will keep the backend as is for now but the current limitations will not be addressed.

[sergisiso]

Maybe "Moving to kwargs options (#2668) will fix this problem" instead.

[sergisiso]

@hiker could this still be an issue?

[hiker]

I think for LFRic, yes. The dependency tools check if there is any 'inconsistent' access (i.e. one field used with different indices, e.g. a(i), and a(i+1)). That's overly careful (depending on indices loop fusion might be valid), but better than wrong code :)

But: for LFRic I don't think we convert the stencil information into accesses that the dependency tools use (in gocean we create 'artificial' accesses, .e.g. a(i,j) and a(i+1,j). You might know that better than me :)

[sergisiso]

OK, I found #2498, @haanahfrost can you restore the TODO but point to this issue instead.

[sergisiso]

I would leave: # Provide a stop_point, otherwise the call itself is returned

[hiker]

This has been fixed, and can be removed:

diff --git a/tutorial/training/transformation/3.6-sympy/solution/dataflow.py b/tutorial/training/transformation/3.6-sympy/solution/dataflow.py
index 4c7ea7ffd1..40ea419f9d 100755
--- a/tutorial/training/transformation/3.6-sympy/solution/dataflow.py
+++ b/tutorial/training/transformation/3.6-sympy/solution/dataflow.py
@@ -104,13 +104,7 @@ for var in varinfo:
             # statement. Find if and where it was previously
             # written:
             node = all_accessed[read_var][0].node
-            # TODO: #3143 atm requires to provide a stop_point,
-            # otherwise the call itself is returned.
-            if not isinstance(node, Statement):
-                stop_position = node.ancestor(Statement).abs_position
-            else:
-                stop_position = node.abs_position
-            chain = DefinitionUseChain([node], stop_point=stop_position)
+            chain = DefinitionUseChain([node])
             sig = node.get_signature_and_indices()[0]
             all_prev = chain.find_backward_accesses()[sig]

[sergisiso]

#1772 / #1453 -examples/psyad/eg2/Makefile. Manual sed rewrite still present.

This is fixed, remove both comments and make the Makefile target.

 ${ADJ_KERNEL_F90_FILE}: ${ADJ_KERNEL_X90_FILE}
     cp $< $@

#365 - nemo.py, valid_kernel.

Replace issue number to #3341

#309 — kernels_trans.py. Traced to #399→#485, but DependencyTools never adopted here.

Remove sentence mentioning this issue.

#2236 -test_lfric_adjoint.py, lfric_adjoint.py. Successor #2347 also closed; workaround still present.

Update issue number to 3546

#2596 - adjoint_visitor.py. Points at a merged PR; author unsure if related to #1946 or #2729.

Update issue number to 3546

#1556 - test_lfric_adjoint.py

Update to 1595

#753 - ~8 sites across doc/developer_guide/psykal.rst, lfric.py, tests.

Update to 1618

#1272 - goloop_test.py.

Replace #1272 with "a previous bug"

#1504 -fparser2.py. Reverted from an earlier premature deletion flagging rather than removing.

Replace with 1471

#1807 - lfric_adjoint.py.

Replace with 3546

#2823 - call_test.py. Reverted from an earlier premature deletion flagging rather than removing.

This has been fixed, remove todo and all the xfail code below:

    # We can't tell whether 'domain%get_start(i)' is an array access
    # or a function call. We currently, dangerously, assume it is the former.
    if not vam.has_read_write(Signature("i")):
        pytest.xfail(reason="TODO #2823 - potential array accesses/function "
                     "calls are always assumed to be array accesses. This is "
                     "unsafe.")