made load_stac nicer to mismatch in band names #755

VictorVerhaert · 2025-03-31T17:03:01Z

Note: warning message could be made clearer but out of inspiration

VictorVerhaert · 2025-03-31T17:04:34Z

there is one test test_run_local_udf_from_file_netcdf_with_context that should not have changed in behaviour that fails.

openeo/rest/datacube.py

…make-bands-metadata-flexible

CHANGELOG.md

openeo/metadata.py

openeo/rest/datacube.py

tests/rest/test_connection.py

openeo/rest/datacube.py

tests/rest/test_connection.py

soxofaan · 2025-04-08T10:53:43Z

tests/rest/test_connection.py

+        cube = con120.load_stac(str(stac_path), bands=["B01", "B02"])
+        assert cube.metadata.band_names == ["B01", "B02"]
+        assert (
+            "The specified bands ['B01', 'B02'] are not a subset of the bands [] found in the STAC metadata (unknown bands: ['B01', 'B02']). Using specified bands as is."


... not a subset of the bands [] ...

hmm I'm a bit confused here: if there is no band dimension, doesn't the add_dimension() above at least define a band with name None ? How can the list of bands be empty?

openeo-python-client/openeo/metadata.py

Lines 619 to 624 in 4d350c4

def get_band_metadata(eo_bands_location: dict) -> List[Band]:

# TODO: return None iso empty list when no metadata?

return [

Band(name=band["name"], common_name=band.get("common_name"), wavelength_um=band.get("center_wavelength"))

for band in eo_bands_location.get("eo:bands", [])

]

receives an empty dict and returns an empty lists which causes bands to be empty in

openeo-python-client/openeo/metadata.py

Line 676 in 4d350c4

band_dimension = BandDimension(name="bands", bands=bands)

So I guess there isn't a case where the metadata has no bands dimension. I'll remove

openeo-python-client/openeo/rest/datacube.py

Lines 447 to 452 in 4d350c4

if not metadata.has_band_dimension():

metadata = metadata.add_dimension(

name="bands",

type="bands",

label=None,

)

So I guess there isn't a case where the metadata has no bands dimension

not yet, but that will be possible in the future with #743

- prepare logic for absent band dimension (related to #743) - Introduce CubeMetadata.ensure_band_dimension (instead of confusing rename_labels usage) - avoid assumption about name of band dimension - more test coverage

soxofaan · 2025-04-08T21:30:57Z

While chewing on this PR, I decided to finetune this PR with 83ef8eb

so feel free to inverse-review :)

VictorVerhaert · 2025-04-09T11:31:16Z

openeo/metadata.py

@@ -423,6 +423,25 @@ def add_dimension(self, name: str, label: Union[str, float], type: Optional[str]
            dim = Dimension(type=type or "other", name=name)
        return self._clone_and_update(dimensions=self._dimensions + [dim])

+    def ensure_band_dimension(
+        self, *, name: Optional[str] = None, bands: List[Union[Band, str]], warning: str


I'm not really a fan of having a function in metadata that takes a warning as an argument just to log it. Logging could happen outside of this function imo

yeah I was hesitant about that,
but forcing the caller to think of a warning message is to make clear that this function should only be used exceptionally

in that case I would rename it to replace_band_dimension or even set_band_dimension as ensure sound like running some checks.
Still not a big fan of the warning argument, but if we keep I'd at least make it optional.

I'm quickly working on a commit that also retains the existing bands so we don't lose fields like wavelength_um

I'll add a note to the documentation to better explain this design

Note that for cases like this where there is doubt about the API that is added, simply making it somehow 'private' or 'internal' API can help to be future proof.

tests/rest/test_connection.py

…ps://github.com/Open-EO/openeo-python-client into issue752-load-stac-make-bands-metadata-flexible

VictorVerhaert · 2025-04-09T15:20:21Z

expanded the ensure_bands_dimension to preserve existing bands. Back to you for the reviews

soxofaan · 2025-04-09T16:22:11Z

expanded the ensure_bands_dimension to preserve existing bands

I'm curious why that is necessary? When you get into the ensure_bands_dimension code path, it implies you don't trust the original band metadata (or there is none), so why doing all the effort to still copy stuff?

Is this to support a particular use case?

VictorVerhaert · 2025-04-10T08:20:02Z

I figured we could keep as much data as possible, but indeed not for a specific use case.
Should I remove it to keep things simple or leave it as is (which is basicly just supporting another case)

This reverts commit 1868c22.

VictorVerhaert · 2025-04-14T11:39:03Z

reverted my last commit as indeed it complicates things for a cause we will not encounter
Made nesure_bands_dimension "private"

…make-bands-metadata-flexible

- prepare logic for absent band dimension (related to #743) - Introduce CubeMetadata.ensure_band_dimension (instead of confusing rename_labels usage) - avoid assumption about name of band dimension - more test coverage

soxofaan · 2025-04-14T13:55:13Z

merged in 5e77f00

(FYI: I did a rebase first to eliminate the commit-revert pair and to get a cleaner history)

made load_stac nicer to mismatch in band names

2c04f73

VictorVerhaert requested a review from soxofaan March 31, 2025 17:03

VictorVerhaert linked an issue Mar 31, 2025 that may be closed by this pull request

load_stac - prevent setting metadata to None due to missing band labels #752

Closed

added changelog entry

66ea570

soxofaan reviewed Apr 1, 2025

View reviewed changes

openeo/rest/datacube.py Outdated Show resolved Hide resolved

VictorVerhaert added 2 commits April 2, 2025 14:52

made warning message more explicit

1446f44

Merge remote-tracking branch 'origin/master' into issue752-load-stac-…

56e0731

…make-bands-metadata-flexible

VictorVerhaert requested a review from soxofaan April 2, 2025 13:51

soxofaan reviewed Apr 3, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

openeo/metadata.py Show resolved Hide resolved

openeo/rest/datacube.py Outdated Show resolved Hide resolved

openeo/rest/datacube.py Outdated Show resolved Hide resolved

soxofaan reviewed Apr 3, 2025

View reviewed changes

tests/rest/test_connection.py Outdated Show resolved Hide resolved

tests/rest/test_connection.py Show resolved Hide resolved

openeo/rest/datacube.py Outdated Show resolved Hide resolved

processed pr comments

4d350c4

VictorVerhaert requested a review from soxofaan April 4, 2025 15:12

soxofaan reviewed Apr 8, 2025

View reviewed changes

VictorVerhaert and others added 2 commits April 8, 2025 13:18

removed caplog.clear and removed add_dim(bands) case

5d9b6d4

Issue #752/#755 anticipate absence of band dimension

83ef8eb

- prepare logic for absent band dimension (related to #743) - Introduce CubeMetadata.ensure_band_dimension (instead of confusing rename_labels usage) - avoid assumption about name of band dimension - more test coverage

VictorVerhaert commented Apr 9, 2025

View reviewed changes

soxofaan and others added 3 commits April 9, 2025 16:42

Issue #752/#755 document warning arg of ensure_band_dimension

c344264

added preservation of bands in ensure_bands_dimension

1868c22

Merge branch 'issue752-load-stac-make-bands-metadata-flexible' of htt…

371eee5

…ps://github.com/Open-EO/openeo-python-client into issue752-load-stac-make-bands-metadata-flexible

VictorVerhaert requested a review from soxofaan April 9, 2025 15:20

VictorVerhaert added 2 commits April 14, 2025 13:30

Revert "added preservation of bands in ensure_bands_dimension"

9c5a6d5

This reverts commit 1868c22.

made ensure_band_dimension private

df7f662

Merge remote-tracking branch 'origin/master' into issue752-load-stac-…

494b44a

…make-bands-metadata-flexible

soxofaan added a commit that referenced this pull request Apr 14, 2025

Issue #752/#755 document warning arg of ensure_band_dimension

d89aff5

soxofaan closed this Apr 14, 2025

soxofaan deleted the issue752-load-stac-make-bands-metadata-flexible branch April 14, 2025 13:55

soxofaan mentioned this pull request Apr 14, 2025

load_stac - prevent setting metadata to None due to missing band labels #752

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

made load_stac nicer to mismatch in band names #755

made load_stac nicer to mismatch in band names #755

VictorVerhaert commented Mar 31, 2025

VictorVerhaert commented Mar 31, 2025

soxofaan Apr 8, 2025

VictorVerhaert Apr 8, 2025

soxofaan Apr 8, 2025

soxofaan commented Apr 8, 2025 •

edited

Loading

VictorVerhaert Apr 9, 2025

soxofaan Apr 9, 2025

VictorVerhaert Apr 9, 2025

soxofaan Apr 9, 2025

jdries Apr 14, 2025

VictorVerhaert commented Apr 9, 2025

soxofaan commented Apr 9, 2025 •

edited

Loading

VictorVerhaert commented Apr 10, 2025

VictorVerhaert commented Apr 14, 2025

soxofaan commented Apr 14, 2025

	def get_band_metadata(eo_bands_location: dict) -> List[Band]:
	# TODO: return None iso empty list when no metadata?
	return [
	Band(name=band["name"], common_name=band.get("common_name"), wavelength_um=band.get("center_wavelength"))
	for band in eo_bands_location.get("eo:bands", [])
	]

	if not metadata.has_band_dimension():
	metadata = metadata.add_dimension(
	name="bands",
	type="bands",
	label=None,
	)

made load_stac nicer to mismatch in band names #755

made load_stac nicer to mismatch in band names #755

Conversation

VictorVerhaert commented Mar 31, 2025

VictorVerhaert commented Mar 31, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soxofaan commented Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VictorVerhaert commented Apr 9, 2025

soxofaan commented Apr 9, 2025 • edited Loading

VictorVerhaert commented Apr 10, 2025

VictorVerhaert commented Apr 14, 2025

soxofaan commented Apr 14, 2025

soxofaan commented Apr 8, 2025 •

edited

Loading

soxofaan commented Apr 9, 2025 •

edited

Loading