search

stfc / PSyclone

Domain-specific compiler and code transformation system for Finite Difference/Volume/Element Earth-system models in Fortran
BSD 3-Clause "New" or "Revised" License
104 stars 27 forks source link

(Closes #2636) alter LFRic PSy-layer to lookup nlevels for each kernel #2678

Closed arporter closed 1 week ago

arporter commented 2 months ago

As the title says. I've also renamed 'DynCellIterators' to 'LFRicCellIterators'. It could/should be moved out to domain/lfric too but that can be done as a final step after the bulk of the reviewing is done.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.86%. Comparing base (5833320) to head (084e61f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2678 +/- ## ======================================= Coverage 99.86% 99.86% ======================================= Files 353 354 +1 Lines 49061 49082 +21 ======================================= + Hits 48997 49018 +21 Misses 64 64 ```

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

arporter commented 2 months ago

This is now ready for a first review. It's a small change but has affected a lot of tests because the name of nlayers in the generated PSy-layer code has changed. It's core LFRic functionality so could be one for @hiker or @TeranIvy. Integration tests are green.

arporter commented 1 month ago

Thanks @hiker. This is ready for another look now. Since I've slightly changed the implementation I haven't performed the split on this iteration. I'll do that once you're happy (and re-launch the integration tests too).

hiker commented 4 weeks ago

@arporter, the coverage reports one line missing. Not sure if I missed that previously, or if this is a new change??

Otherwise it's looking good, I am happy to do the split and fix the missing test.

arporter commented 4 weeks ago

I've now done the move and fixed the resulting circular imports (plus the uncovered line). I also had to write a few tests to get coverage of the 'new' file. I've triggered the integration tests. Should be ready for another look.

arporter commented 4 weeks ago

The NEMO5 integration test failed (run crashed) but I don't think that's the fault of this PR.