Repository navigation
Addition of multiscale pyramids for cloud/snow probability and classification layers - #313
Closed
shollendEODC wants to merge 56 commits into
Closed
shollendEODC wants to merge 56 commits into
shollendEODC wants to merge 56 commits into
Conversation
Adds eopf_geozarr.cpm, an integration package for eopf-cpm (the eopf package, available via the new 'cpm' extra on Python >= 3.13): - GeoZarrWriter, registered in CPM's EOWriterRegistry as engine "geozarr" (by-name only: cpm_zarr already claims .zarr targets in by-target discovery). Routes products to the Sentinel-2 optimized pipeline or the generic pipeline based on the declared CPM product:type, with a structural fallback. Zarr v3 only; rejects unsupported modes, formats, and options loudly. - an 'eopf convert-geozarr' subcommand exposed through CPM's eopf.cli entry-point group, delegating to eopf.store.convert with the engine preset, enabling one-pass SAFE -> GeoZarr conversion. - routing helpers importable without eopf installed. convert_s2/convert_s2_optimized now skip backing-store validation for DataTrees with no zarr backend (e.g. built in memory by the CPM SAFE reader) instead of crashing in get_zarr_group. Verified against eopf 3.0.0 on Python 3.13: engine registration, convert-geozarr discovery in the eopf CLI, and an end-to-end write_datatree(engine="geozarr") producing the flat r10m..r720m pyramid from an S2 example store. Assisted-by: ClaudeCode:claude-fable-5
Fixes the confirmed findings from a multi-agent adversarial review: - write() now runs every argument-derivable check (target type, remote scheme, w- existence, pipeline routing, required 'groups') BEFORE the destructive mode="w" removal of an existing store. Previously a pure options mistake (e.g. missing groups) deleted the user's previous product and then failed with nothing written. - accept compute=True (rejecting only compute=False): CPM's staged-output path (staging.py) injects compute=True when a remote Dask client is active, so the strict kwargs rejection broke the exact --stage-output flow the writer's own error message recommends. - validate_write_options now applies the target checks (type, remote scheme), so CPM's triggering preflight rejects unsupported targets at parse time instead of at write time. - routing: only S02MSIL1C/S02MSIL2A products go to the S2-optimized pipeline; other MSI types (S02MSIL0_, S02MSIRAW) lack the reflectance pyramid structure and now route generic. - eopf_geozarr.cpm raises a clear ImportError explaining the Python >= 3.13 floor when eopf is missing (pip install 'eopf-geozarr[cpm]' silently installs nothing on 3.12). - _validate_s2_input: cover both branches with tests; also skip validation for zarr-v3-backed trees (the S2 input model reads Zarr v2 only and raised TypeError on CPM 3.x products). Verified on Python 3.13 with eopf 3.0.0: 42 tests pass, including new regression tests for the destroy-then-fail ordering and compute handling. Assisted-by: ClaudeCode:claude-fable-5
- test-cpm job: installs the cpm extra (eopf pinned via uv.lock) on Python 3.13 and runs the CPM writer/routing tests on every push/PR, so they no longer silently skip in CI. - test-cpm-latest job: weekly scheduled run against the newest published eopf release to catch upstream contract drift (writer registry, staging, CLI entry-point loading) independently of PR failures. - new test: invoke the real `eopf convert-geozarr` console script via subprocess on a synthetic S2 store, covering entry-point discovery, click option wiring, and convert()'s reader dispatch through to the geozarr engine. Assisted-by: ClaudeCode:claude-fable-5
PR EOPF-Explorer#212 (Sentinel-3 OLCI GeoZarr export) merged to main after EOPF-Explorer#238 was branched, leaving select_pipeline unable to reach it: OLCI products fell into the generic pipeline and failed on the missing 'groups' option. Adds S3_OLCI_PRODUCT_TYPE_PREFIXES and looks_like_sentinel3_olci to routing.py, structural fallback included -- oa01_radiance is a data variable of the measurements node, not a child node, unlike the S2 r{N}m pyramid groups. Wires convert_olci_optimized into the writer with a new s3_olci_optimized flag and an output_grid option, kept additive to the existing write() signature (s2_optimized is untouched) since that's the EOWriter entry point other CPM code calls into.
* ci: run CI on the cpm-geozarr branch The workflow only triggered on main, so no work on the cpm-geozarr feature branch ever got checks -- including EOPF-Explorer#247, which is 20 commits and +29105/-3763 across 23 files. The Samples Service runs this branch in its September GeoZarr production track, so changes reached a production pipeline unchecked. Closes EOPF-Explorer#253 * style: wrap the resolved_spatial_chunk conditional to satisfy ruff format Introduced by the OLCI routing commit, which renamed the local `pipeline` variable to `selected_pipeline` and pushed the line past the limit. It went unnoticed because the branch had no CI -- which is what this PR fixes.
* init project setup * adding and harmonising the chunking/sharding functionality across measuremetn and non-measurement groups - allows the contuniation of chunking behaviour from input eopf datatree instead if overwriting by force - scale_offset logic not touched * addign nan masking for coarsening operation for multiscale calualtation (only for s2_optimising, later extend!) response to issue EOPF-Explorer#246 * addign rechunking after readin to circumvent chunking at read-in (which is hardwired to eopf/store/mappers) * adding L1C injected bands for mutliscale calculation -> with rechukning of added bands to spatial_chunk size * readding prior launch script * implementing changes recommended by claxn and d-v-b, mainly docs, cleanup, simplification * adressing change requests, improving chunk workflow * simplifing encoding retrieval for (all and) 1d arrays * hotfix: adding stac metadata to root level was previously not inlcuded -> copying root metadata to new tree root now * adding docstrings * working on the tests, frustrated with test layout * adding zarr v3 test samples * change of the data_api tests * removing test structure which is not adhering to s2 specs, also 'notebook' scripts referenced in tests didnt seem to exist * changing the s2 zarr v3 structure and all associated tests. Also removing all emojis from output messages * adapting to new zarr layout, changed function calls, and multiscale calculation changes * removing emojis * adding checks to add metadata to arrays if keep_scale_offset=False, modifying checks for s2 structure to align with tests, addig docs * fixing enum to strenum to allow detection of S2type in simple == comparison * applying changes to validate pre-commit pyright issues * addressign requested chagnes for stAc metadata addition adn fill_value fallbacks, removing 'experimental_scale_offset_codec'functionality for encoding2 * small issue of not rechuking in tests * removing addition of multiscales to stac asset metadata, other small commetns * adding _FillValue to encoding to allow xarray decode_cf to correctly read zarr store in abscence of eopf-xarray reader, removed large commented section --------- Co-authored-by: Emmanuel Mathot <emmanuel.mathot@gmail.com>
…ng pr247 (EOPF-Explorer#267) * adding single /measurement/refectance group to stac as requested during pr247 * adressing comments for crs reuse and proj_attrs_for_crs use * adding test to incorporate new functionalities and expanding comments
Brings in S1 GRD RTC ingestion (EOPF-Explorer#216), release 0.11.0 (EOPF-Explorer#235) and dependency bumps (EOPF-Explorer#268, EOPF-Explorer#269). Conflicts: tests/conftest.py keeps the cpm-geozarr v3 S2 fixtures and adds main's S1 RTC fixtures; uv.lock re-locked from main's lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in the spatial:bbox pixel-edge fix (EOPF-Explorer#275, fixes EOPF-Explorer#266). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ition of generalised functionality to utils which are not used in other functions
…p traversal across metadata-ingestion mdoes
…ozarr) but allows a path to which is needed for rechunking
…-main-feat-generic-conversion-s1slc
…justed error messge from writer
… due to utils.create_uniform_encoding, Requesting input to check if tests or encoding needs to be adapted (in sight of new scale/offset codec)
…float32 per default if keep_scale_offset=False;
…s an atomic product due to SLC size
…ate_write_options', removing assert and introducing new pass-through error
…esult_datatree, rechunk_dataset_for_encoding, get_chunking_for_encoding), keeping functions from s2 processing (simple_root_consolidation, stream_write_dataset->stream_write_s2dataset)
…eeds to be discussed
…_store_root_geo_metadata() which uses shapely for bounds calculation from geojson geometry
…te_multiscale_from_datatree_snapshot, tests/test_s2_multiscale.py::test_create_multiscale_from_datatree_behavior failing duie to missing geozarr attrs before MS calculation
…s to mutliscale ingestion
…ersion-s2-cld-snw-scl-MS
6 tasks
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…hich is more stable than accessign spatial:xx directly
Collaborator
Author
|
Tests passed, would be ready for a review @emmanuelmathot @claxn @lhoupert |
Contributor
|
Thanks @shollendEODC! I checked this PR against the experimental multiscales product I looked at on 8 Oct.
Maybe two additional requests:
Test product: could you upload an L2A and an L1C built from this head (ideally the same T33UWQ and T27WXN inputs but other are fine too) under a new prefix? So I can run our acceptance checks (https://github.com/EOPF-Explorer/data_model_validation_tests/tree/main/acceptance) on the three new groups, render each pyramid, and report back here. :-) |
Collaborator
Author
Yeah I think this would make sense, I just used the functionality already provided in the scripts.
Yeah, that would also make sense! I will add it to the list of todos for this branch. Of course, test products will be uploaded! I will notify you here once they are uploaded. |
claxn
approved these changes
Oct 14, 2026
claxn
left a comment
claxn
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for extending the current scope of the Sentinel-2 products, @shollendEODC ! I left a few optional comments.
Collaborator
Author
|
@lhoupert Files are currently being uploaded to s3://continuous-integration/eopf-geozarr/branches/eat-s2-cld-snw-scl-MS/ and are processed with this commit 655e5e6 These samples include ONLY S1 and S2 products, as S3 would route through the Please refer to my last comment in #249 for updates regarding the formalization of rules for attributes and consolidation. |
|
|
||
| SCL_ATTRS = { | ||
| "flag_values": SCL_VALUES, | ||
| "flag_meanings": SCL_LABELS, |
Contributor
There was a problem hiding this comment.
CF requires flag_meanings as one string with the values separated by spaces. The existing b00 attrs also use a list, so this is consistent. Which form does the explorer expect?
Collaborator
There was a problem hiding this comment.
Tagging @lhoupert to be sure everything is provided as expected.
Contributor
|
Thanks @shollendEODC for the PR, see my comments and suggestions above |
Collaborator
|
@emmanuelmathot @lhoupert Could you please add me to the project as a member? @shollendEODC is on holiday and I will take over for the time being. Unfortunately, I can't push to his fork, which is why I would like to either contribute directly to branches in this repo or if you prefer I can also work in my own fork. I will then open a new PR implementing the review comments. Sorry for the inconvenience! |
Contributor
|
@claxn You should have received an invite for write access |
claxn
added a commit
that referenced
this pull request
Oct 27, 2026
…classification layers (supersedes #313) (#318) * feat(cpm): add CPM writer plugin registering the geozarr engine Adds eopf_geozarr.cpm, an integration package for eopf-cpm (the eopf package, available via the new 'cpm' extra on Python >= 3.13): - GeoZarrWriter, registered in CPM's EOWriterRegistry as engine "geozarr" (by-name only: cpm_zarr already claims .zarr targets in by-target discovery). Routes products to the Sentinel-2 optimized pipeline or the generic pipeline based on the declared CPM product:type, with a structural fallback. Zarr v3 only; rejects unsupported modes, formats, and options loudly. - an 'eopf convert-geozarr' subcommand exposed through CPM's eopf.cli entry-point group, delegating to eopf.store.convert with the engine preset, enabling one-pass SAFE -> GeoZarr conversion. - routing helpers importable without eopf installed. convert_s2/convert_s2_optimized now skip backing-store validation for DataTrees with no zarr backend (e.g. built in memory by the CPM SAFE reader) instead of crashing in get_zarr_group. Verified against eopf 3.0.0 on Python 3.13: engine registration, convert-geozarr discovery in the eopf CLI, and an end-to-end write_datatree(engine="geozarr") producing the flat r10m..r720m pyramid from an S2 example store. Assisted-by: ClaudeCode:claude-fable-5 * fix(cpm): apply adversarial-review findings to the writer plugin Fixes the confirmed findings from a multi-agent adversarial review: - write() now runs every argument-derivable check (target type, remote scheme, w- existence, pipeline routing, required 'groups') BEFORE the destructive mode="w" removal of an existing store. Previously a pure options mistake (e.g. missing groups) deleted the user's previous product and then failed with nothing written. - accept compute=True (rejecting only compute=False): CPM's staged-output path (staging.py) injects compute=True when a remote Dask client is active, so the strict kwargs rejection broke the exact --stage-output flow the writer's own error message recommends. - validate_write_options now applies the target checks (type, remote scheme), so CPM's triggering preflight rejects unsupported targets at parse time instead of at write time. - routing: only S02MSIL1C/S02MSIL2A products go to the S2-optimized pipeline; other MSI types (S02MSIL0_, S02MSIRAW) lack the reflectance pyramid structure and now route generic. - eopf_geozarr.cpm raises a clear ImportError explaining the Python >= 3.13 floor when eopf is missing (pip install 'eopf-geozarr[cpm]' silently installs nothing on 3.12). - _validate_s2_input: cover both branches with tests; also skip validation for zarr-v3-backed trees (the S2 input model reads Zarr v2 only and raised TypeError on CPM 3.x products). Verified on Python 3.13 with eopf 3.0.0: 42 tests pass, including new regression tests for the destroy-then-fail ordering and compute handling. Assisted-by: ClaudeCode:claude-fable-5 * ci(cpm): run CPM integration tests in CI and add a CLI end-to-end test - test-cpm job: installs the cpm extra (eopf pinned via uv.lock) on Python 3.13 and runs the CPM writer/routing tests on every push/PR, so they no longer silently skip in CI. - test-cpm-latest job: weekly scheduled run against the newest published eopf release to catch upstream contract drift (writer registry, staging, CLI entry-point loading) independently of PR failures. - new test: invoke the real `eopf convert-geozarr` console script via subprocess on a synthetic S2 store, covering entry-point discovery, click option wiring, and convert()'s reader dispatch through to the geozarr engine. Assisted-by: ClaudeCode:claude-fable-5 * feat(cpm): route Sentinel-3 OLCI products to the optimized pipeline PR #212 (Sentinel-3 OLCI GeoZarr export) merged to main after #238 was branched, leaving select_pipeline unable to reach it: OLCI products fell into the generic pipeline and failed on the missing 'groups' option. Adds S3_OLCI_PRODUCT_TYPE_PREFIXES and looks_like_sentinel3_olci to routing.py, structural fallback included -- oa01_radiance is a data variable of the measurements node, not a child node, unlike the S2 r{N}m pyramid groups. Wires convert_olci_optimized into the writer with a new s3_olci_optimized flag and an output_grid option, kept additive to the existing write() signature (s2_optimized is untouched) since that's the EOWriter entry point other CPM code calls into. * ci: run CI on the cpm-geozarr branch (#254) * ci: run CI on the cpm-geozarr branch The workflow only triggered on main, so no work on the cpm-geozarr feature branch ever got checks -- including #247, which is 20 commits and +29105/-3763 across 23 files. The Samples Service runs this branch in its September GeoZarr production track, so changes reached a production pipeline unchecked. Closes #253 * style: wrap the resolved_spatial_chunk conditional to satisfy ruff format Introduced by the OLCI routing commit, which renamed the local `pipeline` variable to `selected_pipeline` and pushed the line past the limit. It went unnoticed because the branch had no CI -- which is what this PR fixes. * Cpm geozarr eodc changes (#247) * init project setup * adding and harmonising the chunking/sharding functionality across measuremetn and non-measurement groups - allows the contuniation of chunking behaviour from input eopf datatree instead if overwriting by force - scale_offset logic not touched * addign nan masking for coarsening operation for multiscale calualtation (only for s2_optimising, later extend!) response to issue #246 * addign rechunking after readin to circumvent chunking at read-in (which is hardwired to eopf/store/mappers) * adding L1C injected bands for mutliscale calculation -> with rechukning of added bands to spatial_chunk size * readding prior launch script * implementing changes recommended by claxn and d-v-b, mainly docs, cleanup, simplification * adressing change requests, improving chunk workflow * simplifing encoding retrieval for (all and) 1d arrays * hotfix: adding stac metadata to root level was previously not inlcuded -> copying root metadata to new tree root now * adding docstrings * working on the tests, frustrated with test layout * adding zarr v3 test samples * change of the data_api tests * removing test structure which is not adhering to s2 specs, also 'notebook' scripts referenced in tests didnt seem to exist * changing the s2 zarr v3 structure and all associated tests. Also removing all emojis from output messages * adapting to new zarr layout, changed function calls, and multiscale calculation changes * removing emojis * adding checks to add metadata to arrays if keep_scale_offset=False, modifying checks for s2 structure to align with tests, addig docs * fixing enum to strenum to allow detection of S2type in simple == comparison * applying changes to validate pre-commit pyright issues * addressign requested chagnes for stAc metadata addition adn fill_value fallbacks, removing 'experimental_scale_offset_codec'functionality for encoding2 * small issue of not rechuking in tests * removing addition of multiscales to stac asset metadata, other small commetns * adding _FillValue to encoding to allow xarray decode_cf to correctly read zarr store in abscence of eopf-xarray reader, removed large commented section --------- Co-authored-by: Emmanuel Mathot <emmanuel.mathot@gmail.com> * adding single /measurement/refectance group to stac as requested during pr247 (#267) * adding single /measurement/refectance group to stac as requested during pr247 * adressing comments for crs reuse and proj_attrs_for_crs use * adding test to incorporate new functionalities and expanding comments * first explorative introduction of the generic geozarr processing. Addition of generalised functionality to utils which are not used in other functions * swiching consolodation methodology * updating the processing of consolidated metadata and harmonizing group traversal across metadata-ingestion mdoes * removal of functions from utils.py which are not essential for this PR * adding new routing which keeps the original routing same (allows ->geozarr) but allows a path to which is needed for rechunking * small restructure of Error raising in writer, adjust test to catch adjusted error messge from writer * updating docstrings and removing unneeded root consolidation function * adding tests for generic converter logic, two tests are still failing due to utils.create_uniform_encoding, Requesting input to check if tests or encoding needs to be adapted (in sight of new scale/offset codec) * configuring create_uniform_encoding(..) to NOT cast uint16 inputs to float32 per default if keep_scale_offset=False; * adding S1_GRDH zarrv3 fixture instead of SLC as it can be contained as an atomic product due to SLC size * removal of unneccassary fucntions from utils.py * adding 'generic_rechunker' flags to eopf 'convert-geozarr' and 'validate_write_options', removing assert and introducing new pass-through error * removing from s2_multiscales.py and all associated changes * removing from s2_multiscales.py and all associated changes - forgot 2 scripts * removing functions from s2 processing (optimization_summary, create_result_datatree, rechunk_dataset_for_encoding, get_chunking_for_encoding), keeping functions from s2 processing (simple_root_consolidation, stream_write_dataset->stream_write_s2dataset) * reverting uv.lock changes to 39b06d6 * fixing tests-changes induced by changes in code, NOT fixing as this needs to be discussed * add shapely to dependency so tests dont fail as they call utils.write_store_root_geo_metadata() which uses shapely for bounds calculation from geojson geometry * switch to new encoding logic, move of functions to new file to keep encoding atomic and separated, inclusion of new encoding logic and coordinate are not compressed anymore in create_uniform_encoding() * adding scipy to dependencies for SLC conversion, also updating uv.lock for this change * modifying tests to reflect switch to scale_offset_codec * removal of * removal of * moving 'valid_min' and 'valid_max' to encoding_utils.py/_decode_packed() to only be removed during flaot decoding and keep for int dtypes * remodelling of chunking functions, small additions * adding to for harmonization * adressing requested cahgnes in utils.py/updated_root_consolidaiton to improve readabillity, small associated refactors in other files due to function name changes * removing check which was only valid for enable_sharding=True * removing raster:scale and raster:offset * regenerating S2 fixtures * simplifying create_uniform_encoding() for variable and coordinate chunk/sharding * small updates to utils and tests to reflect changes * not working, erroneous commit for s2 cld/snw/scl-multiscales and measureemtn multiscales harmonisation * not working, erroneous commit for s2 cld/snw/scl-multiscales and measureemtn multiscales harmonisation * fixing bugs in MS calculation, tests/test_s2_multiscale.py::test_create_multiscale_from_datatree_snapshot, tests/test_s2_multiscale.py::test_create_multiscale_from_datatree_behavior failing duie to missing geozarr attrs before MS calculation * first addition of cld&snw and l1c&l2a_classification multiscales * adding ms bands addtion to stac_discovery similar to 6f9476a * small refactrs of s2_multiscale.py, restructure of determine_variable_type() * applying changes to tests and derived changes&bugfixes&simplifications to mutliscale ingestion * adding consolidatin for MS groups * rm shapely, fell through in merge * improving consolidation to only check the datasets that were actually added * deriving transform for lvl 0 mutliscale from _spatial_level_attrs() which is more stable than accessign spatial:xx directly * refactor: aux multiscales are now optional; removed redundant output_path; import fixes; unique variable type function; downsample factor error handling * test: consolidated metadata for aux multiscales and preserving native bands * fix: implemented nearest downsampling of classification * feat: added new MultiscaleSpec class as a new interface to generic_multiscale(...) * added trim comment * fix: eopf-geozarr version added for all products and based on git and installed package version * fix: SCL '_FillValue' addition to encoding --------- Co-authored-by: Davis Vann Bennett <davis.v.bennett@gmail.com> Co-authored-by: Emmanuel Mathot <emmanuel.mathot@gmail.com> Co-authored-by: shollendEODC <samuel.hollendonner@eodc.eu> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: claudio.schein-navacchi <claudio.schein-navacchi@eodc.eu> Co-authored-by: Loïc Houpert <10154151+lhoupert@users.noreply.github.com>
Collaborator
|
Closing this one, since #318 is now merged to main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I expect these to fail, but running the tests here is easier so some fixes will likely have to be done..
Based on: #274
Changes:
MS only created for measurements, now also for these products (Claude generated):
/conditions/mask/l2a_classification(SCL)r20m/quality/probability(cld, snw)r20m/conditions/mask/l1c_classification(b00)r60m/measurements/reflectancer10m_AUX_MULTISCALESgeneric_multiscales()computes multiscales with predefined parameters for each listed product aboveresampling_methodsto only feature those which are actually used,b00passed throughclassificationsimple_root_consolidationonly for S2Further modification of this PT note is eminent.
If tests pass its ready for review @emmanuelmathot @claxn @lhoupert , else I will adjust the code as required.
This PR needs to be implemented after #310
Once #310 is resolved, merge main into this branch before finishing PR