Add functions to calculate cycle closure

[hannahbaumann]

Description

Provide a brief description of the PR's purpose here.

Todos

• Enable the comparison of multiple methods within the same FEMap object

Status

• Ready to go

[pep8speaks]

Hello @hannahbaumann! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-11-06 12:35:21 UTC

[codecov]

Codecov Report

❌ Patch coverage is 98.71795% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.53%. Comparing base (3a81321) to head (1838ac4).
⚠️ Report is 99 commits behind head on main.

Files with missing lines	Patch %	Lines
cinnabar/femap.py	97.91%	1 Missing ⚠️
cinnabar/tests/test_plotting.py	96.42%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
+ Coverage   97.35%   97.53%   +0.17%     
==========================================
  Files          22       22              
  Lines        2229     2470     +241     
==========================================
+ Hits         2170     2409     +239     
- Misses         59       61       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[hannahbaumann]

[hannahbaumann]

pre-commit.ci autofix

[IAlibay]

Couple of things

[hannahbaumann]

pre-commit.ci autofix

[IAlibay]

Couple of small things - mainly a question on simple_cyles and maybe if cycle_basis should be used instead?

[IAlibay]

I've been staring at this for a while and it's not immediately clear to me if calling simple_cycles on an undirected graph won't yield the same cycle in both orientation - i.e. [A, B, C] and [A, C, B].

Would this be intended behaviour? If not, should cycle_basis be used instead?

[jthorton]

Wouldn't this show up in test_get_cycle_closure_perfect_cycle, there we check that the resulting dataframe has length 1 so only a single cycle is found.

[hannahbaumann]

It looks like simple_cycles filters those out:

A “simple cycle”, or “elementary circuit”, is a closed path where no node appears twice. In a directed graph, two simple cycles are distinct if they are not cyclic permutations of each other. In an undirected graph, two simple cycles are distinct if they are not cyclic permutations of each other nor of the other’s reversal.
https://networkx.org/documentation/stable/reference/algorithms/generated/networkx.algorithms.cycles.simple_cycles.html

But I could also switch to cycle_basis if that would be better overall!

[jthorton]

Thinking about this more, maybe cycle_basis would be better. Take the following example from the networkx docs:

G = nx.Graph()
nx.add_cycle(G, [0, 1, 2, 3])
nx.add_cycle(G, [0, 3, 4, 5])
list(nx.simple_cycles(G))
> [[0, 1, 2, 3], [0, 1, 2, 3, 4, 5], [0, 3, 4, 5]]

Would we want the values on the smallest possible cycles of the graph? The super cycles formed by combinations of cycles don't tell us anything new do they?

[hannahbaumann]

I think my thought process from back when I was doing that as a grad student was that if we included more cycles (so also the super cycles) the risk of "missing" bad edges would be lower where by missing I mean that some cycles can give a low cycle closure by chance (bad edges but cancelation of errors). And then I included the max_cycle_length since the super large cycles don't seem very meaningful. In addition I had used this to then count how often an edge appears in a bad cycle, and there the super cycles are helpful to pin down the bad egg(s) in the cycle.
But maybe you're right and including those super cycles wouldn't really add that information?

[jthorton]

Okay that makes sense and seems to agree with the observations in https://pubs.acs.org/doi/10.1021/acs.jcim.5c00554 where closing the basis cycles does not mean the larger cycles will close, lets go with simple_cycles and come back to this if we find any issues!

[IAlibay]

It would be good to record this somewhere in an issue - otherwise we might never come back to it / remember why we made this decision.

[IAlibay]

Probably should document this cycle choosing behaviour too somewhere user facing (it'd be ok as a separate issue / PR).

[hannahbaumann]

Opened an issue here: #218

[jthorton]

Thanks @hannahbaumann LGTM!

[IAlibay]

I'll approve but I would like it if we could document some of the decision making on how we gather cycles.

[IAlibay]

Probably should document this cycle choosing behaviour too somewhere user facing (it'd be ok as a separate issue / PR).

[jthorton]

Wait do we want this to be undirected, this will miss self-loops if users run forward and backward edges which might be nice to include in the output?

[hannahbaumann]

As discussed offline, the directed graph would not catch something like this as a cycle: A->B; C->B; C->A.
For now we will leave it as is, but opening an issue that this should be fixed in the future (#219)

[jthorton]

Spoke on slack about this, this does not pick up cycles in the directed case for cycles like A->B; C->B; C->A? we will update the notes of the function to say this does not cover self loops.

[hannahbaumann]

pre-commit.ci autofix