Skip to content

Add support to TiledDataset for missing, irregular or overlapping tiles#487

Merged
Cadair merged 61 commits intoDKISTDC:mainfrom
SolarDrew:tiledds-improvements
Mar 20, 2025
Merged

Add support to TiledDataset for missing, irregular or overlapping tiles#487
Cadair merged 61 commits intoDKISTDC:mainfrom
SolarDrew:tiledds-improvements

Conversation

@SolarDrew
Copy link
Contributor

Changes the array of datasets underlying TiledDataset into a masked array, defaulting to False everywhere (all tiles are valid).

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #487 will not alter performance

Comparing SolarDrew:tiledds-improvements (10e4954) with main (76b7932)

Summary

✅ 10 untouched benchmarks
🆕 2 new benchmarks
⁉️ 1 dropped benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
⁉️ test_tileddataset_repr 1.8 ms N/A N/A
🆕 test_tileddataset_repr[simple-masked] N/A 1.9 ms N/A
🆕 test_tileddataset_repr[simple-nomask] N/A 1.9 ms N/A

@Cadair Cadair marked this pull request as draft January 14, 2025 13:02
@SolarDrew SolarDrew marked this pull request as ready for review January 21, 2025 16:24
@SolarDrew
Copy link
Contributor Author

There are a few places where this can still be improved - if we're happy to make issues for those and punt them then this is good to go pending review. Otherwise I'll come back to it on Monday.

Copy link
Member

@Cadair Cadair left a comment

Choose a reason for hiding this comment

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

👍 This looks good.

I think we should postpone merging this until we have made the corresponding changes to dkist-inventory just to make sure we haven't missed anything.

@SolarDrew SolarDrew force-pushed the tiledds-improvements branch from c8ef224 to a90a49f Compare February 19, 2025 13:33
@SolarDrew
Copy link
Contributor Author

* Tests for `Tileddataset.files.[download | basepath.setter]` etc.

Does this not test the latter?

@SolarDrew
Copy link
Contributor Author

* Tests for `Tileddataset.files.[download | basepath.setter]` etc.

Does this not test the latter?

Apparently not because those lines aren't getting hit.

@Cadair Cadair enabled auto-merge (squash) March 20, 2025 16:16
@Cadair Cadair disabled auto-merge March 20, 2025 16:16
@Cadair Cadair merged commit 0ccb3a4 into DKISTDC:main Mar 20, 2025
22 of 24 checks passed
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