Include preselected samples in sampling results - #2255
lukas-lightly wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughSampling now includes preselected samples in result tags before newly selected samples. It also creates result tags when no additional samples are available. Documentation and tests reflect the updated standard and sequence sampling behavior. ChangesSampling accumulation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The sampling accumulation changes are covered for normal and exhausted-input paths, with no current merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The changelog section is complete, but the change summary and testing sections are too brief. The description does not explain the behavior change, motivation, affected sampling behavior, dependencies, or how to reproduce the tests. Resolution Expand the change summary with the new preselected-sample behavior, the LightlyOne alignment, affected sampling paths, and any issue or motivation. Describe the tests that were added or run, including reproduction instructions and relevant test configuration. State whether dependencies are required.
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d47241864
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| account. The preselected samples influence the strategy and appear before the newly | ||
| selected samples in the result. `n_samples_to_select` is the number of new samples to select. |
There was a problem hiding this comment.
When diverse(..., selected_sequence_length=...) is used, this promise is false: sampling_via_database delegates to sequence_sampling.sampling_via_database_sequences, which still slices off selected_sequence_indices[:len(preselected_indices)] before creating the tag. Consequently a second sequence batch excludes all preselected frames, unlike regular sampling; include that prefix in the sequence result or document the exception explicitly.
Useful? React with 👍 / 👎.
| account. The preselected samples influence the strategy and appear before the newly | ||
| selected samples in the result. `n_samples_to_select` is the number of new samples to select. |
There was a problem hiding this comment.
The persisted sampling result cannot expose the stated prefix order: sampling_helpers.create_result_tag calls add_sample_ids_to_tag_id, which constructs insertion rows from set(sample_ids), and SampleTagLinkTable has no ordering column. The new test only observes the transient argument before this order is discarded, so users querying the result tag cannot rely on preselected samples appearing first; describe them as included, or persist an explicit rank.
Useful? React with 👍 / 👎.
| selected_sample_ids = [ | ||
| input_sample_ids[index] for index in selected_indices[len(preselected_indices) :] | ||
| ] | ||
| selected_sample_ids = [input_sample_ids[index] for index in selected_indices] |
There was a problem hiding this comment.
This now includes preselected IDs in every regular result tag, but the public docstrings for Sampling.metadata_weighting, diverse, deduplicate, annotation_balancing, metadata_balancing, subpart_diversity, and multi_strategies still state that these samples are excluded unless the same tag name is reused. Those docstrings feed the linked API reference, leaving Python users with instructions that contradict the implemented behavior.
Useful? React with 👍 / 👎.
![lightly-fast-track-bot[bot]](https://avatars.githubusercontent.com/in/4182859?s=60&v=4){kind=small}
✅ Fast Track: all required checks passed — auto-approved.
To run the guardrails locally, from Reflects |
3d47241 to
b5a718d
Compare
1 participant
What has changed and why?
This is consistent with LightlyOne.
How has it been tested?
Added tests.
Did you update CHANGELOG.md?
Summary by CodeRabbit
New Features
Documentation