-
Notifications
You must be signed in to change notification settings - Fork 12
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
Sanitise: New transformation sub-package and some refactoring #433
Conversation
672f796
to
1dde540
Compare
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/433/index.html |
039cc1d
to
2a5283d
Compare
This now separates associates and sequence assoction utilities and defines the `SanitiseTransformation` in the sub-pacakge root.
2a5283d
to
d4c7195
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #433 +/- ##
==========================================
+ Coverage 93.13% 93.16% +0.02%
==========================================
Files 200 205 +5
Lines 39702 39832 +130
==========================================
+ Hits 36978 37109 +131
+ Misses 2724 2723 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This will allow custom pipelines to use it independent of the `SanitizeTransformation`.
be64ce2
to
4fb1bc6
Compare
This allows additional targeted expression substitution from config files and pipelines.
4fb1bc6
to
d003c81
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 for this cleanup! Having access to some of these utilities directly via scheduler configs will be very helpful indeed👌 I just have two small comments on extending the test coverage of the SequenceAssociationTransformer
, one of which is optional but I feel the other is an important use case to cover.
else: | ||
for s, lower in zip(arg.shape[:n_dims], arg.dimensions[:n_dims]): | ||
if isinstance(s, RangeIndex): | ||
new_dims += [RangeIndex((lower, s.stop))] |
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.
This is probably quite a common use case, and so could the tests please be extended to cover this?
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, it is and it was not covered in Santeri's original testing. I've slightly refactored his test and added specific cases for this. Please check if this now covers it.
|
||
if not arg.shape: | ||
# Hack: If we don't have a shape, short-circuit here | ||
new_dims = tuple(RangeIndex((None, None)) for _ in dummy.shape) |
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.
[no action] this line is also not covered by the testing, but I suspect we would only arrive here from incomplete enrichment of imported symbols, so probably no pressing need to extend the tests to cover this.
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.
Apologies, this one snuck in by accident (I needed it in one of the dev branches). I've now also added a test for this, so please see if this is ok.
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 a lot for covering the missing lines, this is now good to go! 🙏
This PR creates a new transformation sub-package for sanitisation and then refactors and improves several of the existing utilities for easier inclusion in external pipelines.
In particular it separate associate-handling from sequence-association resolution, and adds a new utility for user-driven expression substitution. Each of those are now available as a single
Transformation
object and have been combined into a combinedSanitisePipeline
, each of which can now be used via external config constructors. The existingSanitiseTransformation
is retained for backward compatibility.In some more detail:
loki.transformations.sanitise
and moved according tests. In this sub-package, existing utilities for associates and sequence association have been separatedTransformer
to avoid awkward map handling. This requires some sub-classing in specific further use-case intransformations.inline
.AssociateTransformation
to offer full or partial associate resolution together with the new associate-merging capabilities.Transformation
, which entails some method renamingSubstituteExpressionTransformation
to expose string-based expression replacementSanitisePipeline
, which is intended as the long-term replacement forSanitiseTransformation
- the latter, however, is retained for external backward compatibility