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

Deprecate Scene.attrs property #1799

Closed
djhoese opened this issue Aug 23, 2021 · 7 comments
Closed

Deprecate Scene.attrs property #1799

djhoese opened this issue Aug 23, 2021 · 7 comments
Assignees
Labels
backwards-incompatibility Causes backwards incompatibility or introduces a deprecation component:scene

Comments

@djhoese
Copy link
Member

djhoese commented Aug 23, 2021

Feature Request

Is your feature request related to a problem? Please describe.
I'm working on #1797 and I'm running into this annoyance where I'd like to get all the sensor names for the loaded datasets. Since there is already a Scene.attrs["sensors"] then I should just get it from that. However, this value is only set when the Scene is created. I propose deprecating Scene.attrs as it is used right now (see below for details). It is not kept up to date and does not reflect the actual loaded data if there is any difference between the reader's metadata and the actual DataArray metadata.

Currently the .attrs dictionary holds:

  • sensors: A set of sensor names gathered from the reader's sensor_names property.
  • start_time: Minimum start_time among all loaded readers
  • end_time: Maximum end_time among all loaded readers

Describe the solution you'd like

  1. Remove the sensors/start_time/end_time default contents of .attrs.
  2. Keep .attrs as an empty dictionary that users can add to (similar to xarray.Dataset), but does not change Satpy behavior at all and will only ever be used as extra metadata if it makes sense (ex. CF writer global attributes maybe?).
  3. Optionally provide .sensors, .start_time, and .end_time properties on the Scene to get the equivalent metadata on-the-fly when it is requested. The values would be gathered by the loaded datasets and would not contact the reader's metadata at all.

Describe any changes to existing user workflow
Are there any backwards compatibility concerns? Changes to the build process? Additional dependencies?

Additional context

Other options:

  1. For point 3 of my above proposed solution: we could fallback to the reader's metadata if no DataArrays are loaded.
  2. Provide a wrapper (Scene.__getattr__) that automatically generates the .attrs metadata when Scene.attrs is requested. Or make .attrs a property (see mention of xarray below).
  3. Provide a helper method that the internals of Satpy (the stuff my PR needs to use) that updates the metadata on ever __setitem__, .load, __delitem__ call.

We have talked in other issues and meetings of making the Scene more like xarray's Dataset object. I think if this deprecation proposal is accepted that we avoid "magic" properties related to existing xarray interfaces like .attrs (my point 2 above).

@djhoese djhoese added component:scene backwards-incompatibility Causes backwards incompatibility or introduces a deprecation labels Aug 23, 2021
@sfinkens
Copy link
Member

sfinkens commented Aug 24, 2021

I totally agree. If the information isn't static, it should become a property. An empty dict for the user's metadata is a nice idea, too.

@mraspaud
Copy link
Member

Good idea with the seeing over this. I would try to mimic as much as possible the behaviour of xr.Dataset objects, as this is where we want to go, right?

@djhoese
Copy link
Member Author

djhoese commented Aug 25, 2021

as this is where we want to go, right?

Yes. I mention that in my last paragraph (I know, it is long). I'll try my best.

@djhoese
Copy link
Member Author

djhoese commented Oct 26, 2021

Question: What should a Scene created from Scene.copy. Should it be empty or be a copy of what the original Scene had? I guess if we're following xarray then it should be empty right?

@djhoese
Copy link
Member Author

djhoese commented Oct 26, 2021

Nevermind, xarray keeps the attrs:

In [1]: import xarray as xr

In [2]: a = xr.DataArray([], attrs={"a": 1})

In [3]: b = a.copy()

In [4]: b
Out[4]:
<xarray.DataArray (dim_0: 0)>
array([], dtype=float64)
Dimensions without coordinates: dim_0
Attributes:
    a:        1

@djhoese
Copy link
Member Author

djhoese commented Oct 26, 2021

Next question, I kind of mention it in my original post, what should start_time/end_time/sensor_names be if there are no loaded datasets? Should it check with the readers or return None? If no readers, should it return None?

@djhoese
Copy link
Member Author

djhoese commented Dec 12, 2021

This was technically closed back in #1797. .attrs no longer holds anything that satpy depends on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatibility Causes backwards incompatibility or introduces a deprecation component:scene
Projects
None yet
Development

No branches or pull requests

6 participants