-
Notifications
You must be signed in to change notification settings - Fork 2
Adding Unit Tests for next release #68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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") |
Copilot
AI
Jun 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is mostly semantic. I fixed regardless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
abdelhafizm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
|
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.