Fix hssm.load_data typing; add hssm.list_data - #1147
Conversation
- `load_data` now requires `dataset` and returns `pd.DataFrame`; the `pd.DataFrame | str` return type forced type-checkers to narrow away a `str` branch that only existed to print the dataset listing - add public `hssm.list_data() -> tuple[str, ...]`, mirroring `hssm.list_models()`, to list built-in datasets instead - `_list_datasets()` is retained for the unknown-dataset error message Refs #1146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- covers the DataFrame return, loading every name from `list_data()`, the ValueError on an unknown dataset, and the TypeError now raised by the no-argument call - there was no dedicated test module for `hssm.datasets` before Refs #1146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- add `docs/api/list_data.md` and a nav entry directly below `hssm.load_data` under "Useful functions" - changelog entry flagging the breaking part: no-argument `load_data()` now raises TypeError instead of returning a listing Refs #1146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
There was a problem hiding this comment.
🟢 Ready to approve
The core API change and repo-wide impact look consistent, with only minor doc/error-message wording nits noted.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR improves the hssm.datasets public API by making hssm.load_data’s typing unambiguous (always a pd.DataFrame) and adding a dedicated hssm.list_data() helper to list available built-in datasets.
Changes:
- Made
hssm.load_data(dataset: str) -> pd.DataFramerequiredatasetand raise on unknown datasets (instead of returning a listing string). - Added public
hssm.list_data() -> tuple[str, ...]and re-exported it fromhssm. - Added a new unit test module plus docs/nav/changelog updates for the new API.
File summaries
| File | Description |
|---|---|
| tests/unit/test_datasets.py | Adds unit coverage for hssm.load_data and new hssm.list_data. |
| src/hssm/datasets.py | Updates load_data typing/behavior and introduces list_data. |
| src/hssm/init.py | Re-exports list_data at the top-level hssm namespace. |
| mkdocs.yml | Adds the new hssm.list_data API page to the docs nav. |
| docs/changelog.md | Documents the typing change + breaking behavior and introduces list_data. |
| docs/api/list_data.md | Adds the mkdocstrings stub page for hssm.list_data. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The Raises section only covered the unknown-dataset-name case; the function also raises when a known dataset's file is missing from the installation. Addresses review comment on src/hssm/datasets.py:52 (#1147) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Use {dataset!r} so names containing whitespace, or an empty string, are
unambiguous in the error message.
Addresses review comment on src/hssm/datasets.py:62 (#1147)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
What
hssm.load_datanow takes a requireddatasetargument and returnspd.DataFrame. The oldpd.DataFrame | strreturn type forced type-checkers (and users) to narrow away astrbranch that existed only to print the dataset listing.hssm.list_data() -> tuple[str, ...]lists the built-in datasets, mirroring the existinghssm.list_models()._list_datasets()is kept private, still used for the descriptive unknown-dataset error message.tests/unit/test_datasets.py— there was no dedicated test module forhssm.datasets.hssm.list_dataAPI page in the nav directly underhssm.load_data; changelog entry.Breaking
hssm.load_data()with no argument now raisesTypeErrorinstead of returning a listing string. No callers in this repo (tests, tutorials, docs) used that form.Note
The issue described
hssm.list_dataas already existing; only the private_list_datasets()did, and it returns a formatted description string. The new public function returns names as a tuple instead, since returning a display string is the same typing smell the issue is about. Descriptions remain in the error message.Verification
uv run pytest tests/unit— 227 passeduv run prek run --files <changed>— ruff, ruff-format, pyrefly, mypy all passBase
Stacked on
1139-reduce-parameters-in-integration-tests(#1142); retarget tomainonce that chain lands.Closes #1146
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
hssm.list_data()to return the available built-in dataset names.Breaking Changes
hssm.load_data()now requires a dataset name.hssm.list_data()to discover available datasets.Tests