Conversation
enryH
force-pushed
the
updt_imputation_module
branch
from
b35530c to
ee2c08d
Compare
- add examples to api examples
- use slightly newer API - many things left to do
- access across rows is also expensive, but not all data needs to be moved twice
- update examples - needs further refinement
There was a problem hiding this comment.
Pull request overview
Updates the imputation functionality and adds new documentation/tests to support the updated API.
Changes:
- Refactors
acore.imputation_analysisimputation functions (KNN + normal-distribution based) with new parameters and updated pandas handling. - Adds pytest coverage for
imputation_KNNandimputation_normal_distribution. - Adds a new “Imputation” API example (both
.pyand.ipynb) and links it from the docs index.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
src/acore/imputation_analysis/__init__.py |
Refactors imputation implementations and public signatures; adds logging/constants. |
tests/test_imputation.py |
Introduces tests for normal-distribution imputation and KNN imputation. |
docs/index.md |
Adds the imputation tutorial to the docs TOC. |
docs/api_examples/imputation.py |
New Jupytext-based imputation tutorial script. |
docs/api_examples/imputation.ipynb |
New notebook version of the imputation tutorial. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "- k Neareast Neighbour imputation can also be used with other types of data\n", | ||
| "- the replacement from the normal distrubtion on the sample level is typical to\n", | ||
| " normally distributed samples from massspectrometer data (in the log2 space)\n", | ||
| "\n", | ||
| "Refers to the [`acore.imputation`](acore.imputation) module." | ||
| ] |
There was a problem hiding this comment.
The notebook intro references the acore.imputation module, but the implementation and imports use acore.imputation_analysis. Please update the referenced module path so the docs don’t link to a nonexistent module.
docs/api_examples/imputation.ipynb
Outdated
| "! does not account for groups\n", | ||
| "imputation_normal_distribution(\n", | ||
| " data=omics_and_y,\n", | ||
| " drop_cols=[group],\n", | ||
| ")" |
There was a problem hiding this comment.
This code cell contains ! does not account for groups which Jupyter will interpret as a shell command and error. It should be a comment (e.g., start with #) so the notebook can execute cleanly.
| ["desired", "cutoff"], | ||
| [(2206.6954572254326, 0.3), (1684.3585255006, 0.6), (837.3284651386293, 0.9)], | ||
| ) | ||
| def test_test_imputation_KNN(example_data, desired, cutoff): |
There was a problem hiding this comment.
Test name has a duplicated prefix (test_test_imputation_KNN). Renaming to test_imputation_KNN would keep naming consistent with other tests and improve readability.
| # | ||
| # Refers to the [`acore.imputation`](acore.imputation) module. | ||
|
|
There was a problem hiding this comment.
The tutorial text references acore.imputation, but the actual module used/imported is acore.imputation_analysis (and there doesn’t appear to be an acore.imputation package). This link/name should be updated so the docs don’t point to a nonexistent module.
There was a problem hiding this comment.
yes, to do. but keep it for the diff for now
Summary
Update imputation module.
List of changes proposed in this PR (pull-request)
diff_regulation_ancova.ipynbtutorial indocs/api_examplesChecks