Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with final model path handling, ensures drug features are properly copied, and adds meta information support to CSV import/export in the FeatureDataset.
- Rename
result_pathtofinal_model_pathto avoid creating the final model directory twice and update calls accordingly. - Introduce
extract_meta_infoinFeatureDataset.from_csvand updateto_csvto preserve original column names or generate fallback feature names. - Expand tests in
test_dataset.pyto cover CSV meta_info extraction, saving, and fallback behavior.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_dataset.py | Extended CSV-related tests to validate meta_info extraction, saving with/without meta_info. |
| drevalpy/experiment.py | Renamed result_path parameter to final_model_path and ensured input data is copied. |
| drevalpy/datasets/dataset.py | Added extract_meta_info parameter in from_csv and updated to_csv to use meta_info. |
drevalpy/datasets/dataset.py
Outdated
Comment on lines
+849
to
+851
| meta_info = None | ||
| if extract_meta_info: | ||
| meta_info = {view_name: list(data_features.columns)} |
There was a problem hiding this comment.
If extract_meta_info is False, meta_info remains None and to_csv will error on self.meta_info.get. Consider initializing meta_info to an empty dict instead of None.
Suggested change
| meta_info = None | |
| if extract_meta_info: | |
| meta_info = {view_name: list(data_features.columns)} | |
| meta_info = {} | |
| if extract_meta_info: | |
| meta_info[view_name] = list(data_features.columns) |
Comment on lines
+639
to
+641
| dataset._meta_info = {} # simulate no meta info | ||
| csv_out_no_meta = temp_dir / "saved_no_meta.csv" | ||
| dataset.to_csv(csv_out_no_meta, id_column="id", view_name=view_name) |
There was a problem hiding this comment.
[nitpick] Accessing _meta_info directly is brittle. Use the public from_csv(..., extract_meta_info=False) API to simulate absence of meta_info instead of touching a private attribute.
Suggested change
| dataset._meta_info = {} # simulate no meta info | |
| csv_out_no_meta = temp_dir / "saved_no_meta.csv" | |
| dataset.to_csv(csv_out_no_meta, id_column="id", view_name=view_name) | |
| csv_out_no_meta = temp_dir / "saved_no_meta.csv" | |
| dataset.to_csv(csv_out_no_meta, id_column="id", view_name=view_name) | |
| dataset = FeatureDataset.from_csv(csv_out_no_meta, extract_meta_info=False) |
This was
linked to
issues
Jun 20, 2025
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.
closes #233 (final model path is created twice)
closes #231 (drug features are not copied for final model path)
closes #232 (add metainfo in load and save csv)
closes #236 (hard-coded pubchem_id, cell_line_name in visualization)
closes #237 (bugs with response_transform)
also changing run_suite test to be with response transformation