Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds and reorganizes unit tests for core modules while cleaning up outdated commented tests.
- Introduces new test suites for graph generation (
gen_*), hybrid clustering, GNN embeddings, downstream task (DPMON), and subject representation. - Removes old commented-out tests for utilities, SMCCNet, metrics, and correlated clustering (to be re-added later).
- Updates test package imports, temporary directory handling, and adapts code to new logging/error behaviors.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_utils.py | Removed obsolete commented-out tests for data summary utilities. |
| tests/test_subject_representation.py | Reformatted and extended SubjectRepresentation unit tests. |
| tests/test_smccnet.py | Deleted entire test file (coverage to be restored later). |
| tests/test_metrics.py | Deleted entire test file (coverage to be restored later). |
| tests/test_hybrid_louvain.py | Refactored setup and added mocks for HybridLouvain. |
| tests/test_graph_gen.py | New test suite covering graph generation utilities. |
| tests/test_gnn_embedding.py | Updated GNNEmbedding tests with patch-based embed/fitting scenarios. |
| tests/test_dpmon.py | Extended DPMON tests with temporary directories and warnings handling. |
| tests/test_correlated_pagerank.py | Deleted entire test file (coverage to be restored later). |
| tests/test_correlated_louvain.py | Deleted entire test file (coverage to be restored later). |
| tests/init.py | Adjusted test imports to include new modules and comment out obsolete. |
| docs/source/index.rst | Updated project title formatting. |
| bioneuralnet/utils/graph.py | Added logger import, warnings, and improved exception handling. |
| bioneuralnet/network_embedding/gnn_embedding.py | Changed empty clinical_data behavior from error to warning. |
| bioneuralnet/downstream_task/subject_representation.py | Enhanced embeddings validation and type checks. |
| README.md | Updated project title wording. |
Comments suppressed due to low confidence (6)
tests/test_utils.py:1
- All tests in tests/test_utils.py have been removed, leaving core utility functions untested. Consider restoring or writing new tests to cover data_summary utilities.
-# import unittest
tests/test_smccnet.py:1
- The entire SMCCNet test file has been deleted. SMCCNet functionality is now untested—add or restore tests to ensure coverage.
-import unittest
tests/test_metrics.py:1
- The metrics test suite was removed entirely, leaving metrics functions unverified. Please reintroduce tests for omics_correlation, evaluate_rf, plotting, etc.
-import unittest
tests/test_correlated_pagerank.py:1
- CorrelatedPageRank tests have been deleted; its behavior is no longer covered by CI. Consider adding back tests for run validity and empty-seed errors.
-import unittest
tests/test_correlated_louvain.py:1
- CorrelatedLouvain tests were removed, removing coverage for quality and DFS outputs. Add tests to ensure clustering correctness.
-import unittest
tests/test_gnn_embedding.py:58
- Missing import for
torchat the top of this test file leads to NameError when referencingtorch.Tensor. Addimport torch.
self.assertIsInstance(embeddings, torch.Tensor)
There was a problem hiding this comment.
Pull Request Overview
This PR adds a suite of new unit tests and updates core utility/error handling, logging, documentation, and CI configurations to prepare for the next release. Key changes include:
- Adding tests for graph generation, data utilities, DPMON downstream task, and dataset loading; updating hybrid-louvain and GNN embedding tests.
- Removing legacy tests for metrics, CorrelatedLouvain, and CorrelatedPageRank, and updating imports in
tests/__init__.py. - Enhancing graph utils with logging, improving
GNNEmbeddingandsubject_representationerror handling, and refining pre-commit/CI scripts.
Reviewed Changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_metrics.py | Entire file deleted—metrics functions now lack dedicated tests |
| tests/test_hybrid_louvain.py | Removed basic test_run, added more focused partition/DFS tests |
| tests/test_graph_gen.py | New tests covering all gen_*_graph utilities |
| tests/test_gnn_embedding.py | Refactored tests, patched embed, removed model params |
| tests/test_dpmon.py | New tests for DPMON, warnings filter, temporary output dir |
| tests/test_dataset_loader.py | New tests for DatasetLoader including error cases |
| tests/test_data_utils.py | New tests for summaries and explore_data_stats output |
| tests/test_correlation_metrics.py | New dedicated tests for omics_correlation, cluster_correlation |
| tests/init.py | Updated imports to include new tests, removed legacy imports |
| bioneuralnet/utils/graph.py | Added logging, improved k bounds, error fallback for Lasso |
| bioneuralnet/network_embedding/gnn_embedding.py | Warn instead of error on empty clinical data |
| bioneuralnet/downstream_task/subject_representation.py | Strengthened embedding/type checks |
| .pre-commit-config.yaml | Tweaked pre-commit hooks for coverage cleanup and test runs |
| docs/source/index.rst | Updated project title |
| .github/workflows/pre-commit.yml | Enabled caching and running pre-commit checks |
Comments suppressed due to low confidence (6)
tests/test_metrics.py:1
- Dedicated unit tests for the metrics module were removed. Consider re-adding tests for
omics_correlation,cluster_correlation,louvain_to_adjacency,evaluate_rf, and all plotting functions to maintain coverage.
-import unittest
tests/test_hybrid_louvain.py:1
- [nitpick] The original
test_runmethod was removed, which reduces coverage for basic HybridLouvain behavior. Consider keeping a simple smoke test to cover default.run()output.
import unittest
tests/test_correlated_pagerank.py:1
- Tests for
CorrelatedPageRankwere deleted. Add back unit tests to cover valid runs and empty-seed error handling.
import unittest
tests/test_correlated_louvain.py:1
- Tests for
CorrelatedLouvainwere deleted. Reintroduce tests to ensure both partition (as_dfs=False) and DataFrame (as_dfs=True) paths are covered.
import unittest
tests/test_gnn_embedding.py:61
- In
test_empty_adjacency_matrix, the instantiation ofGNNEmbeddingexpecting a ValueError isn't wrapped inwith self.assertRaises(ValueError), so the test will error instead of catching the exception.
empty_adj = pd.DataFrame()
.github/workflows/pre-commit.yml:56
- [nitpick] There’s a duplicate commented-out block at the bottom of this workflow. Consider removing the outdated comments to keep the CI config clean.
# - name: Cache pre-commit hooks
| @patch("bioneuralnet.downstream_task.dpmon.run_standard_training") | ||
| @patch("bioneuralnet.downstream_task.dpmon.run_hyperparameter_tuning") |
There was a problem hiding this comment.
The decorator order for patching run_standard_training and run_hyperparameter_tuning is inverted; the function parameters mock_tune and mock_standard are reversed. Swap the decorator order or adjust the parameter order to match.
| @patch("bioneuralnet.downstream_task.dpmon.run_standard_training") | |
| @patch("bioneuralnet.downstream_task.dpmon.run_hyperparameter_tuning") | |
| @patch("bioneuralnet.downstream_task.dpmon.run_hyperparameter_tuning") | |
| @patch("bioneuralnet.downstream_task.dpmon.run_standard_training") |
There was a problem hiding this comment.
I believe this is mostly semantic. I fixed regardless
There was a problem hiding this comment.
Actually, I applied the suggestion and it broke the test. So I put it back how it was.
leaving this open for records
…and on check precommits once
Release v1.0.9
|
Stacked another branch on this branch to update the version number to v1.0.9 |
|
All checks passed. Merging to main. |
Draft PR while I finish Unit Test for remaining classes
todo:
-smccnet
-metrics
For louvain and pagerank I am considering not adding unittest since hybrid calls both of these and is not unlikely that users will use them independently. We can either extend test_hybrid.py or add independent tests for both pagerank and louvain.