feat: configurability for cycle detection

[ParticularlyPythonicBS]

Summary by CodeRabbit

• New Features

  • Added cycle detection controls: cycle_count_limit (default 100; -1 = unlimited, 0 = log first cycle then suppress further reports) and cycle_length_limit (default 1) to control which cycles are reported.

• Documentation

  • Updated docs to explain cycle detection behavior, configuration options, effects on convergence, and guidance for source-tracing/myopic runs.

• Tests

  • Added tests covering cycle filtering, count limits, logging behavior, and stop/report semantics.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9c7842a and 74b3197.

📒 Files selected for processing (1)

• docs/source/quick_start.rst

---

Walkthrough

Adds configurable cycle detection to commodity graph visualization: two new TemoaConfig options (cycle_count_limit, cycle_length_limit), enforces them during cycle reporting, updates docs and sample config, and adds tests verifying logging, filtering, and limit behaviors.

Changes

Cohort / File(s)	Summary
Configuration temoa/core/config.py, temoa/tutorial_assets/config_sample.toml	Added cycle_count_limit (int, default 100) and cycle_length_limit (int, default 1) with validation and sample config entries. TemoaConfig.__repr__ updated to show both fields.
Cycle detection logic temoa/model_checking/commodity_graph.py	Applied cycle length filtering and cycle count enforcement during cycle reporting: skip cycles shorter than cycle_length_limit, respect cycle_count_limit semantics (-1 unbounded, 0 suppress/stop), and emit logs when cycles are detected or when limit reached.
Documentation docs/source/quick_start.rst	Documented cycle detection behavior, convergence implications, and configuration options (cycle_count_limit, cycle_length_limit) in Source Tracing / commodity network section.
Tests tests/test_cycle_limits.py	New tests create graphs with multiple cycles and assert logging/filtering and limit behaviors for cycle_length_limit and cycle_count_limit values (-1, 0, positive).

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Config as TemoaConfig
    participant Graph as CommodityGraph
    participant Logger

    User->>Config: load config (cycle_count_limit, cycle_length_limit)
    User->>Graph: request visualize_graph
    Graph->>Config: read cycle_count_limit, cycle_length_limit
    Graph->>Graph: iterate over detected cycles (generator)
    alt cycle length >= cycle_length_limit
        Graph->>Logger: log "Cycle detected" (include nodes/length)
        Graph->>Graph: increment logged_count
    else
        Graph->>Logger: skip logging (cycle too short)
    end
    alt cycle_count_limit == 0
        Graph->>Logger: log "Cycle detection suppressed / reached zero"
        Graph->>Graph: stop processing cycles
    else if cycle_count_limit > 0 and logged_count >= cycle_count_limit
        Graph->>Logger: log "Cycle detection reached limit"
        Graph->>Graph: stop processing cycles
    else
        Graph->>Graph: continue processing cycles (or unbounded if -1)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding configurability for cycle detection in the commodity graph. It is concise, specific, and directly reflects the primary objective of the pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/220_cycle_dection_configurability

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/source/quick_start.rst`:
- Around line 306-308: Update the documentation for the configuration option
cycle_count_limit to reflect the actual runtime behavior: change the text that
currently says "0 strictly prohibits cycles (stopping execution if any are
found)" to state that setting cycle_count_limit = 0 causes the system to log an
error on the first detected cycle and then suppress further cycle reports for
the remainder of the run, but does not terminate execution. Mention this
behavior alongside the existing descriptions for -1 and positive integers and
ensure the default remains documented as 100.

In `@temoa/core/config.py`:
- Around line 66-67: Add input validation where cycle_count_limit and
cycle_length_limit are accepted (e.g., in the Config/__init__ or factory that
sets these parameters): ensure both are ints, >=1 (or >=0 if zero allowed) and
within any project-specific max if desired, and raise a ValueError with a clear
message if not (e.g., "cycle_count_limit must be a positive int"). Update the
code paths that set cycle_count_limit and cycle_length_limit to perform these
checks before assigning to the instance so invalid types/ranges fail fast with a
descriptive error.

In `@temoa/tutorial_assets/config_sample.toml`:
- Line 55: The inline comment for cycle_count_limit is contradictory: update the
wording for the comment that documents cycle_count_limit so 0 is described as
"strictly disallow" (treat as ERROR) and -1 as "unbounded" (INFO), e.g. change
the phrase “0 = strictly not allow (ERROR, unbounded)” to something like “-1 =
unbounded (INFO), 0 = strictly disallow (ERROR), positive integer = limit” so
it's clear how to configure cycle_count_limit.

In `@tests/test_cycle_limits.py`:
- Around line 53-55: The mocks for generate_commodity_graph,
generate_technology_graph, and nx_to_vis are replacing module-level symbols
without restoring them; update each test block (e.g., where
generate_commodity_graph, generate_technology_graph, nx_to_vis are mocked) to
save the original functions before patching and restore them after the test (or
use monkeypatch.setattr to apply temporary patches). Specifically, capture the
original = cg.generate_commodity_graph / cg.generate_technology_graph /
cg.nx_to_vis, replace with MagicMock for the test, and ensure you restore the
originals in a finally or teardown so other tests (including the similar blocks
at the noted line ranges) are not affected.
- Line 25: The runtime TypeError comes from using subscription on NetworkX types
(e.g., nx.MultiDiGraph[str]) in function/type annotations; replace any
occurrences of nx.MultiDiGraph[str] with plain nx.MultiDiGraph (for example in
the cycle_graph definition) so annotations are not evaluated at import time.
Update all spots where nx.MultiDiGraph[...] is used (including cycle_graph and
the other functions/types flagged in the review) to use nx.MultiDiGraph, or
alternatively wrap those annotations in typing.TYPE_CHECKING or enable postponed
evaluation via from __future__ import annotations if you prefer forward
references.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1644913 and 91a973b.

📒 Files selected for processing (5)

• docs/source/quick_start.rst
• temoa/core/config.py
• temoa/model_checking/commodity_graph.py
• temoa/tutorial_assets/config_sample.toml
• tests/test_cycle_limits.py

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@temoa/core/config.py`:
- Around line 153-156: The validation currently allows booleans because bool
subclasses int; update the checks for cycle_count_limit and cycle_length_limit
to explicitly reject bools (e.g., ensure not isinstance(..., bool) and
isinstance(..., int) or use type(...) is int) and raise the same ValueError if a
boolean is passed so True/False are not accepted as valid integers for
cycle_count_limit or cycle_length_limit.

In `@tests/test_cycle_limits.py`:
- Line 5: Replace the broad typing.Any used in test fixture annotations with
concrete pytest fixture types: change parameters typed as Any to
pytest.LogCaptureFixture for logging fixtures (e.g., caplog) and to
pytest.MonkeyPatch for monkeypatch fixtures (e.g., monkeypatch) in the tests
referenced (the fixtures at lines 5, 45, 87, 121, 153). Remove the now-unused
"from typing import Any" import and ensure pytest is available for these type
names (import pytest if not already). Update the test function signatures (e.g.,
caplog: Any -> caplog: pytest.LogCaptureFixture, monkeypatch: Any ->
monkeypatch: pytest.MonkeyPatch) accordingly.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 91a973b and a6cc955.

📒 Files selected for processing (5)

• docs/source/quick_start.rst
• temoa/core/config.py
• temoa/model_checking/commodity_graph.py
• temoa/tutorial_assets/config_sample.toml
• tests/test_cycle_limits.py

[jdecarolis]

@ParticularlyPythonicBS Can you clarify what the "cycle length" refers to?

[ParticularlyPythonicBS]

@ParticularlyPythonicBS Can you clarify what the "cycle length" refers to?

Its the number of unique vertices in a cycle. So if you look at the examples of simple_cycles example output here, it is the length of the sublists.

This is the standard graph theoretic definition of the length of a cycle

Let me know if you think the definition should be added to the docs

[jdecarolis]

@ParticularlyPythonicBS Thanks for the explanation. I do think it would be helpful to include the definition. So if cycle length =1 by default, the code will look for cycles all the way down to a single self loop?

[ParticularlyPythonicBS]

@ParticularlyPythonicBS Thanks for the explanation. I do think it would be helpful to include the definition. So if cycle length =1 by default, the code will look for cycles all the way down to a single self loop?

Yep!