Skip to content
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.

Sign up for GitHub

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

Fixes modifier attribute getting replaced instead of merged with existing one when applying modifications #2333

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fixes modifier attribute getting replaced instead of merged with existing one when applying modifications #2333

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c7017a5
fix: merge modifier tuples if existant
Aug 23, 2022
6b1a182
Merge branch 'main' into fix_modifier_attrs
Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion satpy/composites/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
'isneginf', 'isposinf']


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,)))
Comment on lines +44 to +46
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. This is quite bad :-D. I will make it more readable.



class IncompatibleAreas(Exception):
"""Error raised upon compositing things of different shapes."""

Expand Down Expand Up @@ -143,13 +148,15 @@ def apply_modifier_info(self, origin, destination):
dataset_keys = ['name', 'modifiers']
for k in dataset_keys:
if k == 'modifiers' and k in self.attrs:
d[k] = self.attrs[k]
d[k] = merge_tuples(d[k], self.attrs[k])
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def match_data_arrays(self, data_arrays):
"""Match data arrays so that they can be used together in a composite.

Expand Down