ecmwf-ifs / loki

Freely programmable source-to-source translation for Fortran
https://sites.ecmwf.int/docs/loki/
Apache License 2.0
22 stars 11 forks source link

Repo reorganisation: Moving transformations #296

Closed mlange05 closed 2 months ago

mlange05 commented 2 months ago

The next step in our spring clean (issue #253) is to move all the internally support transformations into the core package, alongside the associated tests. This then also merges the loki.transform general utilities into the more custom loki.transformations packages.

I've tried to restrain myself to only move files and fix-up import headers, with a few small exceptions listed in the details below:

And finally, apologies to @MichaelSt98 , as this one might conflict quite a bit with his current stuff... :grimacing:

github-actions[bot] commented 2 months ago

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

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 95.71046% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 95.09%. Comparing base (102dafd) to head (fb56455).

Files Patch % Lines
loki/tools/files.py 15.78% 16 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #296 +/- ## ========================================== + Coverage 94.97% 95.09% +0.12% ========================================== Files 151 165 +14 Lines 32407 35282 +2875 ========================================== + Hits 30779 33552 +2773 - Misses 1628 1730 +102 ``` | [Flag](https://app.codecov.io/gh/ecmwf-ifs/loki/pull/296/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf-ifs) | Coverage Δ | | |---|---|---| | [lint_rules](https://app.codecov.io/gh/ecmwf-ifs/loki/pull/296/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf-ifs) | `96.39% <ø> (ø)` | | | [loki](https://app.codecov.io/gh/ecmwf-ifs/loki/pull/296/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf-ifs) | `95.07% <95.71%> (-0.07%)` | :arrow_down: | | [transformations](https://app.codecov.io/gh/ecmwf-ifs/loki/pull/296/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf-ifs) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ecmwf-ifs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MichaelSt98 commented 2 months ago

Thanks, really nice! I'm happy with it and haven't discovered anything problematic. Nevertheless some comments:

I think in order to be consistent:

Why not?


Me thinking out loud:

I guess that further repo reorganisation like "separate operations (inlinecall, subscript, sum, ...) from the symbols/literals" will be done in a future/separate PR?!

mlange05 commented 2 months ago

@MichaelSt98 Many thanks for the review - to answer your questions:

  1. transformations/build_system/tests/test_transform_dependency.py --> transformations/build_system/test_dependency.py
    transformations/tests/test_transform_derived_types.py --> transformations/tests/test_derived_types.py

    Yes, absolutely, that was an oversight! Will fix in the needed rebase!

  2. transformations/transform_loop.py --> transformations/loop.py
    transformations/transform_region.py --> transfrormations/region.py

    Mostly because I actually thought "transform" to be a good high-level description for "fusion/fission/interchange" and "hoist/extract". Open for suggestions though, as the grouping by target "node type" seemed not scalable to me.

  3. Agree in general on the proposed further sub-division or the transformations package; this will potentially also require some re-jigging of the stuff insied some of those transformation (eg. lots in SCCBase could go into utility) and I simply wanted to keep this PR manageable.

With regards to Transformation/Pipeline moving, this simply creates a clean import chain, as everything in loki.transformations should be "do something to code", and thus depend on the ir/expression and batch sub-packages. Hope this makes sense. :wink:

mlange05 commented 2 months ago

Ok, apologies, this took a bit longer than anticipated. @reuterbal I think I've addressed all comments - many thanks for the suggestions!

I've also tried to address the naming inconsistencies pointed out by @MichaelSt98, but those generated some name clashes in the test base (test_derived_type existed twice). So instead I've opted to rename loki.transformations.derived_type to loki.transformations.transform_derived_types to match the test. I hope this works now, and I think we'll get to tidy this up a little further in a follow-on PR.