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

2091: renaming and moving DynKern to LFRicKern #2100

Merged
merged 68 commits into from
Nov 9, 2023

Conversation

mo-lottieturner
Copy link
Collaborator

@mo-lottieturner mo-lottieturner commented Apr 5, 2023

closes #2091

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (34aac4a) 99.84% compared to head (eac784a) 99.85%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #2100    +/-   ##
========================================
  Coverage   99.84%   99.85%            
========================================
  Files         349      349            
  Lines       47376    47206   -170     
========================================
- Hits        47304    47138   -166     
+ Misses         72       68     -4     
Files Coverage Δ
src/psyclone/domain/lfric/__init__.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/algorithm/lfric_alg.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/arg_ordering.py 100.00% <ø> (ø)
src/psyclone/domain/lfric/kern_call_arg_list.py 100.00% <ø> (ø)
...psyclone/domain/lfric/kern_call_invoke_arg_list.py 100.00% <ø> (ø)
src/psyclone/domain/lfric/kern_stub_arg_list.py 100.00% <ø> (ø)
src/psyclone/domain/lfric/kernel_interface.py 100.00% <ø> (ø)
src/psyclone/domain/lfric/lfric_collection.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_kern.py 100.00% <100.00%> (ø)
...c/psyclone/domain/lfric/lfric_kern_call_factory.py 100.00% <100.00%> (ø)
... and 12 more

... and 2 files with indirect coverage changes

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

@TeranIvy
Copy link
Collaborator

TeranIvy commented Apr 20, 2023

Note: To be reviewed after #2090 is merged - this is the final PR to close #2091.

@TeranIvy
Copy link
Collaborator

#2095 is now merged to master. I'd suggest updating this branch with the master, resolving conflicts and tidying up imports. Please also check the changes for pycodestyle and pylint.

@mo-lottieturner
Copy link
Collaborator Author

@arporter I think I spoke too soon there! I think all of the obvious stuff is fixed, and the checks are running through to the pytest part now. The confusing bit is that I have two test failures, neither of which are in files that use DynKern/LFRicKern:

tests/dynamo0p3_multigrid_test.py: test_cont_field_restrict
tests/domain/lfric/transformations/dynamo0p3_transformations_test.py: test_colour_continuous_writer_intergrid

this is mostly for myself to track where i'm at but also wondering if you or @TeranIvy have any insight?

@arporter
Copy link
Member

The second test failure looks as though the Schedule has changed and we are now attempting to apply a transformation to the wrong node. The best way to see what's going on is to do pytest --lf --pdb. Once in the debugger you can examine the Schedule to see its structure. Probably the test should be generalised to use a walk to find the right node. As for the first failing test, that looks like a simple text change. Again, if you use the --pdb option you can print out the code that is being generated and compare with what the test is expecting.

@mo-lottieturner
Copy link
Collaborator Author

@arporter Fixed! back to you for hopefully final review

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.

Looking good now.
Just waiting on integration tests.

src/psyclone/domain/lfric/lfric_kern.py Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_kern.py Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_kern.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_kern.py Outdated Show resolved Hide resolved
@arporter
Copy link
Member

arporter commented Nov 8, 2023

Hi @mo-lottieturner, I was going to make the minor changes myself but there are a few so I'm going to be good and ask you to do them (while the integration tests run). Then we can get this on.

There are also a couple of Comments from previous reviews that haven't been addressed.

@arporter
Copy link
Member

arporter commented Nov 8, 2023

Integration tests were all green :-)

@mo-lottieturner
Copy link
Collaborator Author

back to you @arporter

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.

All requested changes have been made.
Ref. guide still builds fine.
Will proceed to merge.

@arporter arporter merged commit 74bcfbf into master Nov 9, 2023
11 checks passed
@arporter arporter deleted the 2091_renaming_and_moving_dynkern branch November 9, 2023 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LFRic Issue relates to the LFRic domain ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[LFRic] Renaming and moving DynInvoke and DynKern
3 participants