Skip to content

Changement de la parallélisation des traitements avec rioxarray - #24

Open
cadauxe wants to merge 57 commits into
CNES:mainfrom
cadauxe:xarray_interf
Open

Changement de la parallélisation des traitements avec rioxarray#24
cadauxe wants to merge 57 commits into
CNES:mainfrom
cadauxe:xarray_interf

Conversation

@cadauxe

@cadauxe cadauxe commented May 6, 2025

Copy link
Copy Markdown
Collaborator

Fait référence à l'issue #12 .

refactor: checking that the outputs with xarray are the same than with np
refactor: checking similarity for timeseries np and xarray
docs: Updating html doc with cli --help output
@cadauxe

cadauxe commented May 19, 2025

Copy link
Copy Markdown
Collaborator Author

Une option lors du lancement des tests dans la CI a été rajoutée, on peut maintenant accéder au temps que prend chaque test et comparer si tests mettent plus de temps avec rioxarray qu’avec la classe Windowable.

Voici les conclusions majeures :
test_timeseries_command_line_default : de 25.13s à 58.60s (+33,47s)
test_filtering_command_line_default : de 14.15s à 10.24s (-3,91s)
test_radioindice_command_line_default : de 4.71s à 9.05s (+4,34s)
test_zonalstats_command_line_categorical : de 6.49s à 8.50s (+2,01s)

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This PR includes generated/runtime artifacts such as __pycache__/*.pyc, .coverage, *.egg-info, METADATA/RECORD/WHEEL, docs/_build, and generated test outputs. These are environment-specific and make the 1,992-file diff non-reproducible. Please remove generated artifacts and keep the source/config/test changes only.

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi, the new valid/nodata accounting is off: all_count = np.count_nonzero(~mask) already counts only unmasked pixels, which is the same population as dataset.count(). That makes nodata always zero and valid essentially 1.0 even when the clipped region contains nodata. Could all_count represent all pixels in the geometry instead?

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One band-selection regression: both compute_speed() and _interpolate_xarray() use isel(band=slice(0, len(bands))), so the values in bands are ignored. For example, bands=[2, 4] processes the first two bands rather than bands 2 and 4. Could these paths select the requested band coordinates/indices explicitly?

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi, multi-band category stats are collapsed here. roi_statistics is one dict shared across every band, so entries for a given category/stat are overwritten by the next band, and the function always appends a one-element list. Could the per-band dictionaries be kept separate as in the previous return contract?

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One empty-region regression: the old code returned None statistics when dataset.count() == 0, but that guard is gone. With majority or minority requested, an all-nodata clipped region produces an empty pixel_count and _key_assoc_val() calls max([])/min([]), raising instead of returning an empty statistic. Could the zero-valid-pixel case be restored?

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.

2 participants