Suggested changes for Pipeline planning of Duplicate/Remove #463
Conversation
|
Documentation for this branch can be viewed at https://sites.ecmwf.int/docs/loki/463/index.html |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## nams-pipeline-plan-duplicate-remove #463 +/- ##
=======================================================================
+ Coverage 93.27% 93.31% +0.03%
=======================================================================
Files 223 224 +1
Lines 41674 41846 +172
=======================================================================
+ Hits 38871 39048 +177
+ Misses 2803 2798 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| self.kernels = tuple(kernel.lower() for kernel in as_tuple(kernels)) | ||
| self.duplicate_kernels = tuple(kernel.lower() for kernel in as_tuple(duplicate_kernels)) | ||
|
|
||
| def _create_duplicate_items(self, successors, item_factory, config): |
There was a problem hiding this comment.
I feel like this should be a (more generic) utility that lives outside of this transformation (at least in the future).
There was a problem hiding this comment.
I'd agree here in principle, although this feels like it needs an Item.clone() method first and could/should possibly be done in a follow-up.
There was a problem hiding this comment.
An Item.clone() is something we don't want, though - because you want that to always go through the factory.
| self.kernels = tuple(kernel.lower() for kernel in as_tuple(kernels)) | ||
| self.duplicate_kernels = tuple(kernel.lower() for kernel in as_tuple(duplicate_kernels)) | ||
|
|
||
| def _create_duplicate_items(self, successors, item_factory, config): |
There was a problem hiding this comment.
I'd agree here in principle, although this feels like it needs an Item.clone() method first and could/should possibly be done in a follow-up.
reuterbal
added
the
ready to merge
reuterbal
merged commit 9e3acce
into
nams-pipeline-plan-duplicate-remove
These are my suggested changes for #462, as mentioned. This looks like everything's different, but it is mostly moving conceptually things to different places and streamlining the implementation where necessary.
Importantly, I have moved the ItemFactory into a separate file as it was growing a bit. This is the first commit on this branch over #462: c1e9fc5
To properly assess the differences to @MichaelSt98's implementation, you may want to take a look at the diff against this commit: c1e9fc5...nabr-nams-pipeline-plan-duplicate-remove
Conceptually, this implements the following changes that I have also tried to keep in separate commits:
Item.dependencyproperty intoItem.create_dependency_itemsItem.cloneintoItemFactory.get_or_create_item_from_itemDuplicateTrafoandRemoveTrafo