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

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

Merged
merged 19 commits into from
Sep 24, 2024

Conversation

arporter
Copy link
Member

@arporter arporter commented Jul 29, 2024

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.

@arporter arporter marked this pull request as draft July 29, 2024 15:11
@arporter arporter self-assigned this Jul 29, 2024
@arporter arporter added bug LFRic Issue relates to the LFRic domain labels Jul 29, 2024
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

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

@arporter
Copy link
Member Author

arporter commented Aug 1, 2024

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 arporter marked this pull request as ready for review August 1, 2024 09:22
@arporter arporter requested review from hiker and TeranIvy August 1, 2024 09:22
Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

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

A lot of work in fixing up the tests, highly appreciated.
I did find the usage of _nlayers_names a bit confusing (in one case a dict, in others a set). Adding a comment or two to indicate why you can be certain that you have the right data structure ("since ... is called in case of ..., we know that _nlayers_name is a dict") would be useful.

It needs to be brought up to current master (conflicts with master). You also indicated that you want to want to move LFRicCellIterators into domain/lfric - I am happy for you to do this now as well, since there is only a comment that needs to be added.

src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Outdated Show resolved Hide resolved
@arporter
Copy link
Member Author

arporter commented Sep 2, 2024

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
Copy link
Collaborator

hiker commented Sep 3, 2024

@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
Copy link
Member Author

arporter commented Sep 4, 2024

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
Copy link
Member Author

arporter commented Sep 4, 2024

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

Copy link
Collaborator

@hiker hiker left a comment

Choose a reason for hiding this comment

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

All changes look good. Integration test failures unrelated to this PR, so I'll proceed to merge.

@hiker hiker merged commit 70f9793 into master Sep 24, 2024
@hiker hiker deleted the 2636_nlevels_per_kernel branch September 24, 2024 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug LFRic Issue relates to the LFRic domain ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants