Skip to content

Remove dead code paths and tidy lint issues#67

Merged
mvdoc merged 2 commits into
devfrom
claude/paper-code-review-6ab6kp
Jun 10, 2026
Merged

Remove dead code paths and tidy lint issues#67
mvdoc merged 2 commits into
devfrom
claude/paper-code-review-6ab6kp

Conversation

@mvdoc

@mvdoc mvdoc commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Remove code that was unreachable from any production entry point,
verified by an end-to-end parity check (bit-identical flattening
output before and after on a synthetic mesh):

  • algorithm.py: drop run_adaptive_optimization (~300 lines, only
    reachable via the never-enabled adaptive_recovery config flag),
    the always-false use_adaptive dispatch branch, the unused
    compute_distance_error/count_flipped methods, and the orphaned
    compute_normalized_lambdas helper
  • config.py: drop dead adaptive_recovery and n_jobs fields (n_jobs
    was stored but never read; threading is configured from the CLI
    argument before the config file is loaded)
  • distance.py: drop unused heat-method functions (setup_heat_geodesic,
    compute_heat_distance), get_single_k_ring, and compute_graph_distance
  • energy.py: drop the unused edge-list energy path (prepare_edge_list,
    compute_metric_energy_edges, compute_both_energies_edges), the
    superseded compute_area_energy (production uses
    compute_area_energy_fs_v6), and test-only compute_spring_energy,
    compute_log_barrier_area_energy, compute_both_energies,
    compute_total_energy
  • threading.py: drop test-only is_configured/get_effective_threads and
    the tracking global they read
  • cli.py: drop unused cmd_default; core.py: drop unused best_conn;
    viz.py: drop ignored trim parameter of plot_patch
  • move algorithm.py local imports above the TopologyError definition
    and fix remaining ruff findings (unused imports, f-strings without
    placeholders, E712 comparisons)
  • remove tests that existed solely to cover the deleted dead code;
    port the area-energy flip test to compute_area_energy_fs_v6

https://claude.ai/code/session_01XXAPxSRjHzaYagQbgSDaC8

Remove code that was unreachable from any production entry point,
verified by an end-to-end parity check (bit-identical flattening
output before and after on a synthetic mesh):

- algorithm.py: drop run_adaptive_optimization (~300 lines, only
  reachable via the never-enabled adaptive_recovery config flag),
  the always-false use_adaptive dispatch branch, the unused
  compute_distance_error/count_flipped methods, and the orphaned
  compute_normalized_lambdas helper
- config.py: drop dead adaptive_recovery and n_jobs fields (n_jobs
  was stored but never read; threading is configured from the CLI
  argument before the config file is loaded)
- distance.py: drop unused heat-method functions (setup_heat_geodesic,
  compute_heat_distance), get_single_k_ring, and compute_graph_distance
- energy.py: drop the unused edge-list energy path (prepare_edge_list,
  compute_metric_energy_edges, compute_both_energies_edges), the
  superseded compute_area_energy (production uses
  compute_area_energy_fs_v6), and test-only compute_spring_energy,
  compute_log_barrier_area_energy, compute_both_energies,
  compute_total_energy
- threading.py: drop test-only is_configured/get_effective_threads and
  the tracking global they read
- cli.py: drop unused cmd_default; core.py: drop unused best_conn;
  viz.py: drop ignored trim parameter of plot_patch
- move algorithm.py local imports above the TopologyError definition
  and fix remaining ruff findings (unused imports, f-strings without
  placeholders, E712 comparisons)
- remove tests that existed solely to cover the deleted dead code;
  port the area-energy flip test to compute_area_energy_fs_v6

https://claude.ai/code/session_01XXAPxSRjHzaYagQbgSDaC8

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request focuses on cleaning up the codebase by removing unused imports, dead code, and deprecated features across several modules, including the removal of the adaptive optimization routine, unused energy/distance functions, and threading configuration utilities. However, a test correctness issue was identified in autoflatten/tests/test_version.py, where removing the import autoflatten statement disconnects the test from the package it is intended to verify, meaning the fallback version logic is no longer properly tested.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

# Force ImportError when trying to import from _version
import importlib

import autoflatten

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Test Correctness Issue

By removing import autoflatten (previously on line 27), this test is now completely disconnected from the package it is supposed to be testing.

Currently, the test manually replicates the fallback logic in a local try-except block (lines 33-37) and asserts against a local __version__ variable, rather than verifying the actual package-level autoflatten.__version__ fallback behavior.

To make this test meaningful and correct, the test should be refactored to import autoflatten inside the mock context and assert that autoflatten.__version__ == "unknown".

Since the lines containing the assertion are outside the current diff hunk, please consider updating the test in a follow-up or by expanding the changes to:

        with mock.patch.object(
            importlib, "import_module", side_effect=ImportError("mocked")
        ):
            import autoflatten
            assert autoflatten.__version__ == "unknown"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — the test was indeed tautological (it asserted on its own local copy of the fallback logic, even before this PR). Fixed in 90a5391: the test now imports the package inside the mock context and asserts autoflatten.__version__ == "unknown".

One adjustment to the suggested snippet: mocking importlib.import_module doesn't trigger the fallback, because from ._version import version is a from-import statement that never calls importlib.import_module. The fix instead keeps mock.patch.dict(sys.modules, {"autoflatten._version": None}) — a None entry in sys.modules makes the import system raise ImportError on that statement, which exercises the real except branch in autoflatten/__init__.py.


Generated by Claude Code

The previous test replicated the try/except fallback locally and
asserted on its own variable, so it passed regardless of what
autoflatten/__init__.py did. Import the package under the mocked
sys.modules entry and assert on autoflatten.__version__ instead.

Addresses Gemini review feedback on PR #67.

https://claude.ai/code/session_01XXAPxSRjHzaYagQbgSDaC8
@mvdoc mvdoc changed the base branch from main to dev June 10, 2026 00:27
@mvdoc mvdoc merged commit fb03b4d into dev Jun 10, 2026
6 checks passed
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.

2 participants