Refine auto codec selection to be payload-first on full data#153
Open
Refine auto codec selection to be payload-first on full data#153
Conversation
This keeps auto decisions ratio-safe by using sampled bounded checks only as a shortlist and choosing the final winner from full-data payload size with conservative tie-breaking. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR refines automatic compressor selection by making the final decision based on full-dataset payload size first, with deterministic tie-breaking when compressors produce equal payload sizes and errors.
Changes:
- Added a deterministic
compressor_tiebreak_rankto break ties (favoringFFT, thenPolynomial, then others). - Reworked
compress_bestto use sampling only to shortlist candidates, but always evaluate shortlisted compressors on the full dataset and choose by payload → error → tiebreak rank. - Adjusted eligibility handling to ensure at least one compressor result is always selectable, even when none meet the max error constraint.
Comments suppressed due to low confidence (1)
atsc/src/frame/mod.rs:159
partial_cmp(...).unwrap_or(Ordering::Equal)treats NaN errors as equal to numeric errors. When payload sizes tie (and especially in the fallback path where no result meetsmax_error), this can cause a compressor withNaNerror to be preferred over one with a finite error, so the “then error” part of the ordering is not reliably enforced. Consider using a total ordering forf64(e.g.,f64::total_cmp) or explicitly ranking NaN as worst (always greater than any finite error) before applying the tiebreak rank.
let by_error = a.0.error.partial_cmp(&b.0.error).unwrap_or(Ordering::Equal);
if by_error != Ordering::Equal {
return by_error;
}
Self::compressor_tiebreak_rank(a.1).cmp(&Self::compressor_tiebreak_rank(b.1))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Compressor selection improvements:
compressor_tiebreak_rankmethod to provide a deterministic tiebreaker when compressors tie on payload size and error, favoringFFTfirst, thenPolynomial, and others last.Code quality and maintainability:
std::cmp::Orderingimport to support the new comparison logic in compressor selection.