Skip to content

Conversation

luigilcsilva
Copy link
Contributor

In cases where we try to generate the margin cache and it fails because it contains no points, it's safe to ignore the fact that it has no margin when concatenating this catalog with another later, rather than discarding both margins. A new Boolean parameter in the concatenation function, followed by a warning about the possibility of incompleteness in the final catalog's margin cache if not used carefully, solves this use case.

@luigilcsilva
Copy link
Contributor Author

@smcguire-cmu @camposandro My dear reviewers, in addition to the functionality itself, I changed pyproject.toml to avoid failure in the pre-commit command (SKIP=sphinx-build,pytest-check pre-commit run --all-files), but I don't know if I can change this, or if I changed it correctly, so if anything we revert it.

Copy link

github-actions bot commented Sep 26, 2025

Before [5fe1775] After [10b3891] Ratio Benchmark (Parameter)
failed failed n/a benchmarks.time_open_many_columns_all
92.3±1ms 93.3±1ms 1.01 benchmarks.time_kdtree_crossmatch
41.2±0.2ms 41.6±2ms 1.01 benchmarks.time_polygon_search
25.7±0.3ms 25.8±0.2ms 1.00 benchmarks.time_box_filter_on_partition
883±3ms 879±7ms 1.00 benchmarks.time_create_midsize_catalog
591±4ms 592±5ms 1.00 benchmarks.time_open_many_columns_list
5.83±0.01s 5.78±0.03s 0.99 benchmarks.time_create_large_catalog
1.77±0.02s 1.70±0.05s 0.96 benchmarks.time_open_many_columns_default

Some benchmarks failed or their performance regressed significantly.

Copy link

codecov bot commented Sep 26, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.22%. Comparing base (3aa9fb1) to head (afe7806).
⚠️ Report is 21 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   97.19%   97.22%   +0.02%     
==========================================
  Files          46       46              
  Lines        2638     2666      +28     
==========================================
+ Hits         2564     2592      +28     
  Misses         74       74              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@smcguire-cmu smcguire-cmu left a comment

Choose a reason for hiding this comment

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

Thanks for your work again, this looks good! Just a few comments

Copy link
Contributor

@smcguire-cmu smcguire-cmu left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for your work!

Copy link

github-actions bot commented Oct 8, 2025

Before [3aa9fb1] After [ccc8da3] Ratio Benchmark (Parameter)
failed failed n/a benchmarks.time_open_many_columns_all
92.8±1ms 93.1±1ms 1.00 benchmarks.time_kdtree_crossmatch
591±2ms 591±2ms 1.00 benchmarks.time_open_many_columns_list
26.3±0.9ms 25.9±0.4ms 0.99 benchmarks.time_box_filter_on_partition
5.98±0.05s 5.93±0.02s 0.99 benchmarks.time_create_large_catalog
895±10ms 885±10ms 0.99 benchmarks.time_create_midsize_catalog
1.77±0.01s 1.69±0.07s 0.96 benchmarks.time_open_many_columns_default
43.4±1ms 41.6±0.9ms 0.96 benchmarks.time_polygon_search

Some benchmarks failed or their performance regressed significantly.

@luigilcsilva luigilcsilva merged commit 89597b0 into main Oct 8, 2025
13 checks passed
@luigilcsilva luigilcsilva deleted the sep26-2025-implementing-ignore-empty-margin branch October 8, 2025 20:26
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.

Add a parameter to the concatenation function to optionally ignore empty margin caches
4 participants