test(golden_record): re-enable classifier_fast on fixed main#350
Merged
Conversation
PR #323 fixed the fast_class bug that previously marked regular orbits as lost. The temporary GOLDEN_RECORD_SKIP_CASES=classifier_fast escape hatch added in #323 is no longer needed now that main carries the fix, so drop it from the per-test ENVIRONMENT in test/tests/CMakeLists.txt. The SKIP_CASES plumbing in compare_golden_results.sh stays in place for future intentional-divergence bug fixes. Also drop avg_inverse_t_lost.dat from the classifier_fast / classifier_combined file list in compare_golden_results.sh: that file is only written when at least one particle is actually lost. With the fast_class regression fixed, the small classifier_fast test loses zero particles, so neither ref nor cur produces the file. The comparator's "reference missing" check then spuriously fails even though both runs agree byte-for-byte on the files that do exist. make test-regression now runs 13/13 green locally, including the full golden record set (classifier_fast 16.7 s, classifier_combined 16.2 s).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
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
1 participant
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.
Summary
Follow-up to #323. The squash-merged classification fix means main now
matches the output that this branch's
simple.xproduces forgolden_record_classifier_fast, so the temporaryGOLDEN_RECORD_SKIP_CASES=classifier_fastescape hatch added in #323can be removed.
Two small changes:
test/tests/CMakeLists.txt: dropGOLDEN_RECORD_SKIP_CASES=classifier_fastfrom the per-test ENVIRONMENT. The
SKIP_CASESplumbing incompare_golden_results.shstays in place for futureintentional-divergence bug fixes (just don't set the env var
unless a fix needs it).
test/golden_record/compare_golden_results.sh: dropavg_inverse_t_lost.datfrom the classifier file list. That file isonly written when at least one particle is actually lost; with the
fast_class fix the small
classifier_fasttest loses zero particles,so neither ref nor cur writes it. The comparator's "reference file
missing" check then spuriously fails even though both runs agree
byte-for-byte on the files that do exist.
Verification
Full regression suite, against freshly rebuilt main reference (cached
runs/run_mainandsimple_mainrebuilt to pick up #323):Both classifier cases:
Test plan
make test-regressionpasses 13/13 locallygolden_record_classifier_fast(the test fix(classification): fast_class regression + JPP 2023 paper reproduction #323 had to skip) passesgolden_record_classifier_combined(new in fix(classification): fast_class regression + JPP 2023 paper reproduction #323) still passes