-
Notifications
You must be signed in to change notification settings - Fork 13
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
[F2C transpilation] (driver level) convert interface to import #422
Conversation
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/422/index.html |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 93.24% 93.24%
=======================================
Files 220 220
Lines 41105 41137 +32
=======================================
+ Hits 38327 38360 +33
+ Misses 2778 2777 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me. I've left two remarks how to improve the implementation a little bit but that's not mandatory. I'll leave it up to you whether you want to give this a try.
for b in i.body: | ||
if isinstance(b, Subroutine): | ||
if targets and b.name.lower() in targets: | ||
# Create a new module import with explicitly qualified symbol | ||
modname = f'{b.name}_FC_MOD' | ||
new_symbol = Variable(name=f'{b.name}_FC', scope=routine) | ||
new_import = Import(module=modname, c_import=False, symbols=(new_symbol,)) | ||
routine.spec.prepend(new_import) | ||
# Mark current import for removal | ||
removal_map[i] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to use the API properties of Interface
here:
for b in i.body: | |
if isinstance(b, Subroutine): | |
if targets and b.name.lower() in targets: | |
# Create a new module import with explicitly qualified symbol | |
modname = f'{b.name}_FC_MOD' | |
new_symbol = Variable(name=f'{b.name}_FC', scope=routine) | |
new_import = Import(module=modname, c_import=False, symbols=(new_symbol,)) | |
routine.spec.prepend(new_import) | |
# Mark current import for removal | |
removal_map[i] = None | |
for s in i.symbols: | |
if targets and s in targets: | |
# Create a new module import with explicitly qualified symbol | |
new_symbol = s.clone(name=f'{s.name}_FC', scope=routine) | |
modname = f'{new_symbol.name}_MOD' | |
new_import = Import(module=modname, c_import=False, symbols=(new_symbol,)) | |
routine.spec.prepend(new_import) | |
# Mark current import for removal | |
removal_map[i] = None |
interfaces = FindNodes(ir.Interface).visit(routine.spec) | ||
imports = FindNodes(ir.Import).visit(routine.spec) | ||
assert len(interfaces) == 2 | ||
assert len(imports) == 1 | ||
assert imports[0].module.upper() == 'KERNEL_FC_MOD' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to verify here also the name of the imported symbol.
Suggested improvement that uses more API functions:
interfaces = FindNodes(ir.Interface).visit(routine.spec) | |
imports = FindNodes(ir.Import).visit(routine.spec) | |
assert len(interfaces) == 2 | |
assert len(imports) == 1 | |
assert imports[0].module.upper() == 'KERNEL_FC_MOD' | |
assert len(routine.interfaces) == 2 | |
imports = routine.imports | |
assert len(imports) == 1 | |
assert imports[0].module.upper() == 'KERNEL_FC_MOD' | |
assert imports[0].symbols == ('KERNEL_FC',) |
6aaeeb3
to
8fdf8f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many thanks, looks good now. I'll wait for tests to go green, then we can put this in before 0.2.9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, looks great. GTG.
fa94623
to
384e2ec
Compare
Convert interface to kernel calls (e.g.,
interface kernel ...
) to the appropriate import (e.g.,kernel_fc_mod, only: kernel_fc
).