Move methods in cinnabar.stats concerning MLE to the MLEEstimator class by ianmkenney · Pull Request #211 · OpenFreeEnergy/cinnabar

ianmkenney · 2026-05-27T15:06:36Z

No description provided.

ianmkenney · 2026-05-27T15:09:12Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

codecov · 2026-05-27T15:21:12Z

Codecov Report

❌ Patch coverage is 97.31544% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.46%. Comparing base (deef019) to head (090ac2a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
cinnabar/estimators.py	93.10%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #211      +/-   ##
==========================================
- Coverage   97.53%   97.46%   -0.07%     
==========================================
  Files          22       22              
  Lines        2470     2403      -67     
==========================================
- Hits         2409     2342      -67     
  Misses         61       61

☔ 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.

ianmkenney · 2026-05-27T15:34:20Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

…/remove_stats_mle

jthorton · 2026-06-12T10:14:20Z

        )

+    @staticmethod
+    def mle(graph: nx.DiGraph, factor: str = "f_ij", node_factor: Union[str, None] = None) -> (np.ndarray, np.ndarray):


We are free to change this however we want, lets update the factor and node_factor names to be more meaningful or if you think its okay we can leave them.

I've renamed them:

factor -> edge_data_label

node_factor -> node_data_label

jthorton · 2026-06-12T10:16:24Z

+        # populate the edges of the graph along with their computational binding free energies
+        for m in filter(lambda m: m.computational, measurements):
+            if isinstance(m.labelA, ReferenceState):
+                # TODO this is never hit in the tests and should be supported behavior


Good point lets move this to another PR we should be able to support absolute computational values in the solver as well!

Agreed, another PR would be a good place for this

Related to issue #123

jthorton · 2026-06-12T10:19:30Z

+            z[i] += -deltaij / varij
+            z[j] += deltaij / varij
+
+            F_matrix[i, i] += 1 / varij
+            F_matrix[j, j] += 1 / varij
+            F_matrix[i, j] += -1 / varij
+            F_matrix[j, i] += -1 / varij


Nice this should allow for the forward and backward results do you want to add support for that in this PR? We have some example data in #133 so we can add a test we can punt to another if you want though.

Even if we don't test with a full network, I think this should be a halfway decent start for bi-directionality 6fb6c2b

jthorton · 2026-06-12T10:20:00Z

+        return f_i, Finv
+
+    @staticmethod
+    def form_edge_matrix(graph: nx.Graph, label: str, step=None, action=None, node_label=None) -> np.ndarray:


Lets just remove this if its not needed anymore and all tests associated with it!

Removed in 64f09e4

jthorton

Looking great just a couple of things to look at.

Additionally, I removed a comment referencing bayesian_f_ij that doesn't seem to apply to any code in the repository.

The meaning of the "factor" naming convention is unclear. "node_factor", which represents the harmonic wells restraining the movement of individual nodes from a fixed absolute free energy, has been renamed to "node_data_label". "factor", referring to the harmonic restraints between pairs of nodes, has been renamed to "edge_data_label".

ianmkenney · 2026-07-14T13:49:04Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

jthorton · 2026-07-15T12:51:19Z

+                raise ValueError(
+                    f"Multiple edges detected between nodes {m.labelA} and {m.labelB}. "
+                    "MLE cannot be performed on graphs with multiple edges between the "
+                    "same nodes. The results should be combined into a single estimate "
+                    "and uncertainty before performing MLE. "
+                    "See https://cinnabar.openfree.energy/en/latest/concepts/estimators.html"
+                    "#limitations for more details."
+                )


The plan is to fix this at the FEMap level in another PR. Issue - #232

jthorton

Thanks @ianmkenney this looks great we can handle the FEMap fixes in a nother PR.

IAlibay

This is amazing!

Everything looks good, but I am going to block over the one test - it should be an easy fix.

IAlibay · 2026-07-16T10:22:11Z

+        # if we have bidirectional edge results we need to raise an error as they can not be used with MLE
+        # track the edges we have seen
+        edges = []
+        for a, b in graph.edges:
+            edge_name = (a, b) if str(a) < str(b) else (b, a)
+            edges.append(edge_name)


Suggested change

# if we have bidirectional edge results we need to raise an error as they can not be used with MLE

# track the edges we have seen

edges = []

for a, b in graph.edges:

edge_name = (a, b) if str(a) < str(b) else (b, a)

edges.append(edge_name)

Is this leftover code that needs removing?

IAlibay · 2026-07-16T10:24:14Z

Suggested change

Does this need removing?

IAlibay · 2026-07-16T10:52:22Z

+def test_mle_repeated_edge():
+    graph = nx.DiGraph()
+    graph.add_edge(0, 1, f_ij=1.0, f_dij=0.5)
+    graph.add_edge(0, 1, f_ij=1.5, f_dij=0.1)  # repeated edge
+    output_absolutes, _ = MLEEstimator.mle(graph, edge_data_label="f_ij", node_data_label="f_i")
+    output_absolutes -= output_absolutes[0]
+    assert output_absolutes[1] > 1.25
+    assert output_absolutes[1] < 1.5


I don't think this does what the test claims it do do.

If you just add an edge it willl overwrite the previous entry. See the notes here: https://networkx.org/documentation/stable/reference/classes/generated/networkx.Graph.add_edge.html

I think you need to use a MultiDiGraph here, so:

Suggested change

def test_mle_repeated_edge():

graph = nx.DiGraph()

graph.add_edge(0, 1, f_ij=1.0, f_dij=0.5)

graph.add_edge(0, 1, f_ij=1.5, f_dij=0.1) # repeated edge

output_absolutes, _ = MLEEstimator.mle(graph, edge_data_label="f_ij", node_data_label="f_i")

output_absolutes -= output_absolutes[0]

assert output_absolutes[1] > 1.25

assert output_absolutes[1] < 1.5

def test_mle_repeated_edge():

graph = nx.MultiDiGraph()

graph.add_edge(0, 1, f_ij=1.0, f_dij=0.5)

graph.add_edge(0, 1, f_ij=1.5, f_dij=0.1) # repeated edge

output_absolutes, _ = MLEEstimator.mle(graph, edge_data_label="f_ij", node_data_label="f_i")

output_absolutes -= output_absolutes[0]

expected = (1.0 / 0.5**2 + 1.5 / 0.1**2) / (1 / 0.5**2 + 1 / 0.1**2)

assert output_absolutes[1] == pytest.approx(expected)

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

IAlibay

lgtm, thanks!

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

ianmkenney · 2026-07-16T14:09:19Z

@IAlibay 184b0ab. This should allow the estimator to fully support parallel edges. This does cause an error upstream for femap expected behavior which hasn't been fully tested yet. Do you have thoughts on this?

IAlibay · 2026-07-16T14:11:38Z

@IAlibay 184b0ab. This should allow the estimator to fully support parallel edges. This does cause an error upstream for femap expected behavior which hasn't been fully tested yet. Do you have thoughts on this?

I don't mind either way, I'm happy for it to be removed with the idea that femap testing / fixes will be dealt with before the next release.

Allowing parallel edges in the MLEEstimator removes the ValueError normally produced by the FEMap.generate_absolute_values method. The test expecting this ValueError has been removed, but does not explicity mean that FEMap supports parallel edges.

ianmkenney · 2026-07-16T14:30:14Z

@IAlibay 184b0ab. This should allow the estimator to fully support parallel edges. This does cause an error upstream for femap expected behavior which hasn't been fully tested yet. Do you have thoughts on this?

I don't mind either way, I'm happy for it to be removed with the idea that femap testing / fixes will be dealt with before the next release.

Perfect, I've removed it from the FEMap testing. It should be addressed after #232.

ianmkenney added 2 commits May 27, 2026 10:45

Move stats.mle to MLEEstimator.mle

fab42e6

Migrate stats tests concerning mle to test_estimators

6ff2f7b

pre-commit-ci Bot and others added 3 commits May 27, 2026 15:09

[pre-commit.ci] auto fixes from pre-commit.com hooks

a388164

for more information, see https://pre-commit.ci

Add news entry for MLE methods migration

beb532e

Remove TODO comment

10a2f53

Update MLE citation

b37796c

[pre-commit.ci] auto fixes from pre-commit.com hooks

bd901c7

for more information, see https://pre-commit.ci

ianmkenney requested a review from jthorton May 27, 2026 15:35

ianmkenney changed the title ~~[DNM] Move methods in cinnabar.stats concerning MLE to the MLEEstimator class~~ Move methods in cinnabar.stats concerning MLE to the MLEEstimator class May 27, 2026

ianmkenney added 3 commits May 28, 2026 08:54

Rework _build_graph_from_measurements

7b49093

Simplify MLE method

4f6f40e

Merge remote-tracking branch 'origin/feat/remove_stats_mle' into feat…

57a640a

…/remove_stats_mle

jthorton reviewed Jun 12, 2026

View reviewed changes

jthorton self-assigned this Jun 15, 2026

ianmkenney added 8 commits July 8, 2026 11:52

Merge remote-tracking branch 'origin/main' into HEAD

5a5eb6c

Ruff format

d97eca0

Fix typing

63d2974

Apply ruff check --fix

807ef88

Remove form_edge_matrix method from MLEEstimator

64f09e4

Test bidirectional deltas

6fb6c2b

Add clarifying comments to MLE factor contributions

3a25160

Additionally, I removed a comment referencing bayesian_f_ij that doesn't seem to apply to any code in the repository.

ianmkenney force-pushed the feat/remove_stats_mle branch from f030064 to 4c61f18 Compare July 9, 2026 13:00

ianmkenney requested a review from jthorton July 9, 2026 19:32

Update tests and docstrings

091e42a

[pre-commit.ci] auto fixes from pre-commit.com hooks

e8b7ebc

for more information, see https://pre-commit.ci

jthorton reviewed Jul 15, 2026

View reviewed changes

Comment thread news/stats_mle_move.rst Outdated

jthorton mentioned this pull request Jul 15, 2026

Test bi-directional and repeated edges in FEMap #232

Closed

jthorton reviewed Jul 15, 2026

View reviewed changes

Apply suggestion from @jthorton

b60875c

jthorton reviewed Jul 15, 2026

View reviewed changes

Comment thread news/stats_mle_move.rst Outdated

Apply suggestion from @jthorton

2839623

jthorton approved these changes Jul 15, 2026

View reviewed changes

IAlibay requested changes Jul 16, 2026

View reviewed changes

ianmkenney and others added 3 commits July 16, 2026 08:37

Remove old edge tracking

ace857d

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Add Kenney, I.M. & Beckstein, O. to MLE citations

ddb01f9

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Use MultiDiGraph instead of DiGraph for testing repeated edges

4f2a1f8

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

IAlibay approved these changes Jul 16, 2026

View reviewed changes

ianmkenney and others added 2 commits July 16, 2026 10:05

MLEEstimator can fully support parallel edges

184b0ab

Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>

Adjust reference title case

1383d6d

ianmkenney added 2 commits July 16, 2026 10:20

Fix docstring formatting

f7f0076

ianmkenney merged commit 7d0ce09 into main Jul 16, 2026
7 of 9 checks passed

ianmkenney deleted the feat/remove_stats_mle branch July 16, 2026 14:34

This was referenced Jul 16, 2026

Allow for computations where both directions of an edge have been calculated #22

Closed

MLE DG estimates are wrong for bidirectional RBFE calculations on ligand network. #133

Closed

Add multi edge tests for femap #233

Merged

Uh oh!

Conversation

ianmkenney commented May 27, 2026

Uh oh!

ianmkenney commented May 27, 2026

Uh oh!

codecov Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ianmkenney commented May 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ianmkenney Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jthorton left a comment

Choose a reason for hiding this comment

Uh oh!

ianmkenney commented Jul 14, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jthorton left a comment

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

ianmkenney commented Jul 16, 2026

Uh oh!

IAlibay commented Jul 16, 2026

Uh oh!

ianmkenney commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 27, 2026 •

edited

Loading

ianmkenney Jul 9, 2026 •

edited

Loading

ianmkenney commented Jul 16, 2026 •

edited

Loading