Skip to content

Conversation

@ramosv
Copy link
Member

@ramosv ramosv commented Jun 2, 2025

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.

Copy link

Copilot AI left a 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 torch at the top of this test file leads to NameError when referencing torch.Tensor. Add import torch.
self.assertIsInstance(embeddings, torch.Tensor)

@ramosv ramosv requested a review from Copilot June 3, 2025 17:08
Copy link

Copilot AI left a 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 GNNEmbedding and subject_representation error 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_run method 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 CorrelatedPageRank were deleted. Add back unit tests to cover valid runs and empty-seed error handling.
import unittest

tests/test_correlated_louvain.py:1

  • Tests for CorrelatedLouvain were 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 of GNNEmbedding expecting a ValueError isn't wrapped in with 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

Comment on lines +43 to 44
@patch("bioneuralnet.downstream_task.dpmon.run_standard_training")
@patch("bioneuralnet.downstream_task.dpmon.run_hyperparameter_tuning")
Copy link

Copilot AI Jun 3, 2025

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.

Suggested change
@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")

Copilot uses AI. Check for mistakes.
Copy link
Member Author

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

Copy link
Member Author

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

@ramosv ramosv marked this pull request as ready for review June 3, 2025 17:43
This was referenced Jun 3, 2025
@ramosv
Copy link
Member Author

ramosv commented Jun 3, 2025

Stacked another branch on this branch to update the version number to v1.0.9

Copy link
Collaborator

@abdelhafizm abdelhafizm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@ramosv
Copy link
Member Author

ramosv commented Jun 3, 2025

All checks passed. Merging to main.

@ramosv ramosv merged commit 1f5fd31 into main Jun 3, 2025
11 checks passed
@ramosv ramosv deleted the unittest-for-next-release branch July 31, 2025 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants