feat: add user-defined custom CV split scripts#444
Merged
Conversation
Allow advanced users to supply create_splits(response_data) scripts validated by test_mode semantics, wired through the experiment loop, CLI, and make-cv-pkls while preserving the existing fold-dict training contract.
Add sphinx docstrings, replace Protocol with Callable, fix isort/black formatting, and document tests so CI pre-commit passes.
Satisfy flake8-darglint DAR101 for pytest fixture parameters.
Expose pipeline split settings via a frozen CustomSplitParams dataclass passed as the second argument to create_splits(response_data, params).
Document private validation helpers with sphinx param/returns/raises sections.
| rf"{result_dir_str}/{dataset}/" | ||
| r"(LPO|LCO|LDO|LTO)/[^/]+/(predictions|cross_study|randomization|robustness)/.*\.csv$" | ||
| rf"{result_dir_str}/{re.escape(dataset)}/" | ||
| r"[^/]+/[^/]+/(predictions|cross_study|randomization|robustness)/.*\.csv$" |
Collaborator
There was a problem hiding this comment.
I think there are some downstream assumptions on this (e.g., line 102 in the same file), but we have to rework the validation, and I don't like the path splitting/ string matching stuff anyway. I am wondering if we should use your manifests also for the non-custom splits and always parse it to determine test mode etc.
Collaborator
|
Thanks, it looks very good! I left a comment, but would also be okay with a wontfix of that, because the viz is to be reworked anyway |
Move built-in and external split logic into drevalpy.datasets.splits with a shared create_and_record_splits path, JSON manifests that record split_label vs test_mode, and explicit result discovery so reports resolve the semantic test mode correctly.
Collaborator
Author
|
I adjusted to use the same manifest creation for both built-in and custom splitters, and cleaned up the visualization a bit. Hope it's clean enough now |
Add missing docstrings and isort fixes for the split provider refactor, and sync with development.
Make tests.datasets a proper package for mypy, skip row-overlap validation on trusted built-in splits, and keep external split validation unchanged.
Collaborator
|
works for me! looks great!! :) so would approve after that mini fix I commented! Thanks!! |
Use len(splits) instead of the requested params value so external scripts that return a different fold count are reflected accurately.
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
2 participants
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
create_splits(response_data)custom split scripts for advanced CV setups (issue Allow custom split creation procedures #407)test_modesemantics (LPO,LCO,LDO,LTO)make-cv-pklswhile keeping the existing fold-dict training contractTest plan
pytest tests/datasets/test_custom_splits.pydrevalpy make-cv-pkls --custom_splitter_path examples/custom_split_lco_fraction.py ...