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.

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

Improve Compositor API --- Compositor looks like a dict but doesn't behave like one #1221

Open
gerritholl opened this issue May 27, 2020 · 1 comment

Comments

@gerritholl
Copy link
Collaborator

Feature Request

Is your feature request related to a problem? Please describe.

When loading a compositor and showing it interactively, it is represented exactly like a dict. However, it doesn't behave at all like a dict, in fact it's not subscriptable. This is confusing:

In [69]: loader = satpy.composites.CompositorLoader()

In [70]: _=loader.load_compositors(["seviri"])

In [71]: cp = loader.get_compositor("snow", ["seviri"])

In [72]: print(cp)
{'calibration': None,
 'level': None,
 'modifiers': None,
 'name': 'snow',
 'optional_prerequisites': [],
 'polarization': None,
 'prerequisites': [DatasetID(name='VIS008', wavelength=None, resolution=None, polarization=None, calibration=None, level=None, modifiers=('sunz_corrected',)),
                   DatasetID(name='IR_016', wavelength=None, resolution=None, polarization=None, calibration=None, level=None, modifiers=('sunz_corrected',)),
                   DatasetID(name='IR_039', wavelength=None, resolution=None, polarization=None, calibration=None, level=None, modifiers=('nir_reflectance',))],
 'resolution': None,
 'standard_name': 'snow'}

In [73]: print(cp["prerequisites"])
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-73-eb4faa196785> in <module>
----> 1 print(cp["prerequisites"])

TypeError: 'GenericCompositor' object is not subscriptable

There doesn't appear to be a documented way to access the properties displayed.

Describe the solution you'd like

My preferred solution would be to change the representation. It could be presented like an AreaDefinition, a Scene, or a namedtuple. Another solution that does not exclude this at all is to support item access like a dictionary would. Since all the keys are valid Python identifiers, it could also have attribute access like a namedtuple.

Describe any changes to existing user workflow

Users would notice the change in representation. If any users depend on the current workflow, they would have to change their code to represent cp.attrs instead of cp directly. I imagine this situation to be uncommon.

Additional context
Apparently I can access the keys using the attrs attribute: cp.attrs["prerequisites"]. However, this behaviour appears to be an undocumented implementation detail (it's not documented at CompositeBase or MetadataObject), so I hesitate to rely on it. I don't know if there is any supported way to access these properties.

There may be some link with #1088. With more DatasetIDs the relevant, uniquely defining properties of a Compositor could be held in its id. is a namedtuple.

@djhoese
Copy link
Member

djhoese commented May 27, 2020

My preferred solution would be to change the representation. It could be presented like an AreaDefinition, a Scene, or a namedtuple

I'm not against this. The reason it is the way it is is probably because it was the simplest thing to do in the early days of Satpy. The thing we'd have to be careful in changing the __repr__ or __str__ is that it provides useful information. It arguably provides too much information now.

Another solution that does not exclude this at all is to support item access like a dictionary would. Since all the keys are valid Python identifiers, it could also have attribute access like a namedtuple.

No thank you 😉
If the __getitem__ is going to be implemented on the class then it should have a real purpose beyond accessing configuration information.

Users would notice the change in representation.

Overall the compositors are a relatively hidden interface due to how little they are used directly by users. I'm not too worried about changing anything about the compositors class as long as __init__ and __call__ doesn't change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants