Fixes modifier attribute getting replaced instead of merged with existing one when applying modifications #2333
Conversation
| def merge_tuples(*t): | ||
| """Merge tuples and tuples of tuples into one tuple.""" | ||
| return tuple(j for i in (t) for j in (i if isinstance(i, tuple) else (i,))) |
There was a problem hiding this comment.
Could you please use longer names for i, j, and t? This is very hard to read. Why the extra parentheses around (t)? When is i not a tuple?
There was a problem hiding this comment.
You are right. This is quite bad :-D. I will make it more readable.
| elif d.get(k) is None: | ||
| if self.attrs.get(k) is not None: | ||
| d[k] = self.attrs[k] | ||
| elif o.get(k) is not None: | ||
| d[k] = o[k] | ||
|
|
||
| d["_satpy_id"] = d["_satpy_id"]._replace(**d) |
There was a problem hiding this comment.
This is extremely scary to me. I think the need for adding this here is because the way you are using modifiers does not have the proper information provided. If I remember correctly, if the traditional Scene-based usage of modifiers the provided DataID to the modifier (the destination in this method maybe?) already has the modifiers tuple updated.
If this new line of code can't be made unnecessary by providing newer/better information to the Modifier object (either in __init__ or during the __call__ then this suggests that maybe some of the magic modifier handling done in the DependencyTree should be moved here as part of the modifiers/composites.
There was a problem hiding this comment.
You @djhoese obviously have a lot more insight in the whole DataID and dependency_tree logic. Actually this modification is 'quite' old and I just came around to make a PR from this. Now that I had a deeper glance into the DataID and the attribute updating by working on #2332 I agree that this most likely is not the right way to do it. To me it just feels like treating the 'symptoms' of the whole situation around DataArray assignment to the scene, DataIDs (and dependency_treee) reflecting the DataArray attributes and vice versa.
I might be wrong but from what I grasp I would say that updating the attributes (except for the _satpy_id) of the DataArray should be handled in the 'processing' part (so the generation of composites/modifiers), but the generation/updating of the DataID and everything that is tied to it should be closer to the __setitem__ in the Scene. This would make it more central and maintainable because it would be decoupled from all these classes which do it their own way right now.
So I think this needs more thought. Should I mark this as work in progress?
There was a problem hiding this comment.
Marking as WIP sounds good. I think I agree that the ID stuff shouldn't happen in the compositor classes, but it may need to be due to Satpy's current design and since we allow use of compositors as class instances rather than only through the Scene.
If a modifier e.g.
SunZenithCorrectoris applied to aDataArray(as in #1490) which already has amodifierattribute instead of replacing the existing attribute modification get merged.