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

Loki_transform: Re-instate argument shape derivation in convert #149

Merged
merged 7 commits into from
Oct 8, 2023

Conversation

mlange05
Copy link
Collaborator

Post-hoc fix to an oversight in PR #142. When removing the dedicated ecphys entry point, we dropped the argument array shape transformation that is used to replace : dimensions with explicitly named dimensions in the surface modules of the code. This PR adds an option (on by default) to the convert entry point that re-enables this behaviour.

@github-actions
Copy link

Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/149/index.html

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #149 (767dc91) into main (19fd1c4) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #149   +/-   ##
=======================================
  Coverage   92.10%   92.10%           
=======================================
  Files          90       90           
  Lines       16628    16628           
=======================================
  Hits        15315    15315           
  Misses       1313     1313           
Flag Coverage Δ
lint_rules 96.22% <ø> (ø)
loki 92.08% <ø> (ø)
transformations 91.25% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Collaborator

@reuterbal reuterbal left a comment

Choose a reason for hiding this comment

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

Uncontroversial and looks good. Sorry for not spotting this on the original PR.

Only question would be whether to opt for a more descriptive option name, like --derive-argument-array-shape? Similarly to --remove-openmp, --removed-derived-args, --trim-vector-sections I find a verb helps to convey the implication.

@@ -104,12 +104,12 @@ def cli(debug):
help="Generate offload instructions for global vars imported via 'USE' statements.")
@click.option('--remove-derived-args/--no-remove-derived-args', default=False,
help="Remove derived-type arguments and replace with canonical arguments")
@click.option('--argument-array-shape/--no-argument-array-shape', default=False,
@click.option('--derive-argument-array-shape/--no-argument-array-shape', default=False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be --no-derive-argument-array-shape now as well? (Sorry, bit of a mouthful)

@reuterbal
Copy link
Collaborator

Rebased over latest main, resolving conflicts. Further CMake refactoring filed as separate PR (#166) against this branch.

@mlange05
Copy link
Collaborator Author

mlange05 commented Oct 8, 2023

Ok, I think this is now GTG for merging once the CI comes back green.

@mlange05 mlange05 merged commit d352220 into main Oct 8, 2023
12 checks passed
@mlange05 mlange05 deleted the naml-convert-argument-array-shape branch October 8, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants