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

Fix re-loading of StaticImageCompositor composites in special cases #2243

Closed
wants to merge 1 commit into from

Conversation

simonrp84
Copy link
Member

As mentioned in my comment on #2242, re-loading a composite with no pre-requisites fails if the user hasn't loaded any satellite data first:

# Example with SEVIRI, substitute whatever data you have
scn = Scene(['D:/sat_data/SEV/MSG3-SEVI-MSG15-0100-NA-20160326144242.057000000Z-NA.nat'], reader='seviri_l1b_native')
scn.load(["_night_background"])
print("LOADED")
scn.load(["_night_background"])
print("LOADED AGAIN")

In the above code, the second load command will fail with an error as for the _night_background composite the sensor will be None.

In this PR, I fix the problem by - optionally - allowing a sensor to be passed to StaticImageCompositor via the YAML file. This will then be stored in the composite and prevents the error described above.

@@ -419,12 +419,14 @@ composites:
_night_background:
compositor: !!python/name:satpy.composites.StaticImageCompositor
standard_name: night_background
sensor: 'viirs'
Copy link
Collaborator

Choose a reason for hiding this comment

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

That doesn't seem right. Why VIIRS?

Copy link
Member Author

Choose a reason for hiding this comment

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

The night background image is derived from VIIRS DNB data.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess an alternative would be to set the sensor to generic or something like that, but I figure we might as well set it to the correct thing...

@gerritholl
Copy link
Collaborator

Why do composites need a sensor?

@djhoese
Copy link
Member

djhoese commented Oct 26, 2022

As mentioned on slack, in the multi-sensor case a set can be used {'abi', 'glm'}. In the static image case where it doesn't really make sense then I think an empty set or None should be allowed, but I'm not sure it is heavily tested (as you've discovered). I don't know if adding a sensor to the static image composites is the right solution. I'd rather we find a way to fix Satpy handling the case where sensor is None.

@gerritholl
Copy link
Collaborator

Should sensor in this case be None or should there simply not be a sensor attribute at all — similar to how some composites remove the units and wavelength attributes?

If sensor can be a set of the sensors making up the composite, then one could argue that wavelength could also be a set of WavelengthRange objects in for an RGB.

@djhoese
Copy link
Member

djhoese commented Oct 26, 2022

Should sensor in this case be None or should there simply not be a sensor attribute at all — similar to how some composites remove the units and wavelength attributes?

Not sure. I think my hope for a sensor value of None was to avoid the few locations in the code where data_arr.attrs['sensor'] might be used instead of data_arr.attrs.get('sensor'). I'd be OK with removing the sensor though.

If sensor can be a set of the sensors making up the composite, then one could argue that wavelength could also be a set of WavelengthRange objects in for an RGB.

I'm not sure. Multiple sensors refer to the data making up the image. In that sensor multiple wavelengths make sense. However, multiple wavelengths should probably not be in an unordered set as I'm not sure how helpful that is after the fact. The more important thing is that the rest of Satpy won't understand what multiple wavelength ranges means. It is part of the DataID and is used to provide a "unique" identifier for a DataArray. What use case do you have for having the list/set of wavelength ranges that went into an RGB?

@gerritholl
Copy link
Collaborator

If sensor can be a set of the sensors making up the composite, then one could argue that wavelength could also be a set of WavelengthRange objects in for an RGB.

I'm not sure. Multiple sensors refer to the data making up the image. In that sensor multiple wavelengths make sense. However, multiple wavelengths should probably not be in an unordered set as I'm not sure how helpful that is after the fact. The more important thing is that the rest of Satpy won't understand what multiple wavelength ranges means. It is part of the DataID and is used to provide a "unique" identifier for a DataArray. What use case do you have for having the list/set of wavelength ranges that went into an RGB?

Right; a set would be wrong here, a list would be more appropriate, because order matters and values are not guaranteed to be unique.

I don't have a strong use case for retaining the units information. Nor am I quite sure what the use case is of having a set of sensors in the sensor attribute.

@djhoese
Copy link
Member

djhoese commented Oct 26, 2022

The sensor set is mostly to make enhancements happy since they can be organized in per-sensor files.

@mraspaud
Copy link
Member

mraspaud commented Nov 9, 2022

@simonrp84 what is the status of this? I see tests are failing, is it something that we need to fix here?

@djhoese
Copy link
Member

djhoese commented Nov 9, 2022

I'll clarify my feelings from above, I don't think this is the right fix for the original issue. I think Satpy should be fixed to handle a sensor value of None or it not existing.

@simonrp84
Copy link
Member Author

Fair enough, that makes sense - but I don't know the best way to fix satpy for handling sensor=None so I'll leave that to someone else unfortunately. Shall I close this or leave it open until an alternaitve solution is found?

@djhoese
Copy link
Member

djhoese commented Nov 9, 2022

Let's close this. I've made #2264 from your comment on #2242 and assigned both to me. I'm not sure when I'll get to them, but I think they are two separate issues so I wanted to record them that way.

@djhoese djhoese closed this Nov 9, 2022
@simonrp84 simonrp84 deleted the fix_static_comp branch November 9, 2022 15:44
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

Successfully merging this pull request may close these issues.

Composites without prerequisites should be always loadable
4 participants