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

2263 splitting LFRic scalar args #2264

Merged
merged 12 commits into from
Sep 21, 2023
Merged

Conversation

oakleybrunt
Copy link
Collaborator

@oakleybrunt oakleybrunt commented Aug 15, 2023

closes #2263

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (65db1a8) 99.84% compared to head (a106c3e) 99.84%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2264   +/-   ##
=======================================
  Coverage   99.84%   99.84%           
=======================================
  Files         342      343    +1     
  Lines       46069    46077    +8     
=======================================
+ Hits        45998    46006    +8     
  Misses         71       71           
Files Changed Coverage Δ
src/psyclone/domain/lfric/__init__.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_invoke.py 100.00% <100.00%> (ø)
src/psyclone/domain/lfric/lfric_scalar_args.py 100.00% <100.00%> (ø)
src/psyclone/dynamo0p3.py 99.81% <100.00%> (-0.01%) ⬇️

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

Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

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

Nice work! There are some minor comments and clean-ups (see inline).

Tests with compilation work and examples pass. I didn't check documentation in this instance, but grepping for LFRicScalarArgs only returns source and test changes so I think we're fine here.

Also, the branch needs updating with master. I'd advise doing that in a separate commit to review corrections.

src/psyclone/domain/lfric/__init__.py Outdated Show resolved Hide resolved
src/psyclone/dynamo0p3.py Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_scalar_args.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_scalar_args.py Outdated Show resolved Hide resolved
src/psyclone/domain/lfric/lfric_scalar_args.py Outdated Show resolved Hide resolved
@TeranIvy TeranIvy added reviewed with actions LFRic Issue relates to the LFRic domain and removed under review labels Sep 13, 2023
Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

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

All review comments are addressed. Tests with compilation pass except for an unrelated test. It is not the fault of this PR and it is exposed by using GCC 11.2.0 compiler which is stricter in syntax.
Documentation builds fine. Examples run.
After updating the user guide and changelog I will proceed to merge the branch.

Copy link
Collaborator

@TeranIvy TeranIvy left a comment

Choose a reason for hiding this comment

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

CI passed, proceeding to merge.

@TeranIvy TeranIvy merged commit 156480e into master Sep 21, 2023
11 checks passed
@TeranIvy TeranIvy deleted the 2263_splitting_LFRicScalarArgs branch September 21, 2023 09:58
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 under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Splitting LFRicScalarArgs from src/psyclone/dynamo0p3.py
2 participants