Skip to content

Avoid self-loops when building expanded neighborhood graph#105

Open
dafeda wants to merge 3 commits into
equinor:mainfrom
dafeda:no-self-loops
Open

Avoid self-loops when building expanded neighborhood graph#105
dafeda wants to merge 3 commits into
equinor:mainfrom
dafeda:no-self-loops

Conversation

@dafeda

@dafeda dafeda commented Feb 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

No description provided.

@dafeda dafeda changed the title No self loops Avoid self-loops when building expanded neighborhood graph May 4, 2026
@dafeda dafeda marked this pull request as ready for review May 4, 2026 10:13
@dafeda dafeda self-assigned this May 4, 2026
@larsevj

larsevj commented May 4, 2026

Copy link
Copy Markdown
Collaborator

Was it intentional to commit the lockfile?

@dafeda

dafeda commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

Was it intentional to commit the lockfile?

Nothing I do with lockfiles is intentional!

larsevj
larsevj reviewed May 4, 2026
View reviewed changes

@larsevj larsevj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see the function does not seem to be used or tested anywhere, but might be useful in the future?

@dafeda

dafeda commented May 4, 2026

Copy link
Copy Markdown
Collaborator Author

I see the function does not seem to be used or tested anywhere, but might be useful in the future?

fit_precision_cholesky_approximate is used in ert. But yeah, perhaps we should add a test...

@larsevj

larsevj commented May 4, 2026

Copy link
Copy Markdown
Collaborator

I see the function does not seem to be used or tested anywhere, but might be useful in the future?

fit_precision_cholesky_approximate is used in ert. But yeah, perhaps we should add a test...

Oh, I see. Then there should definitely be tests for it...

dafeda added 2 commits May 4, 2026 15:06
@dafeda
Avoid redundant self-loops in expanded graph
878fe73 
single_source_shortest_path_length includes the source node at distance 0, which was adding self-loops to G_expanded.

This does not change current fitting behavior because optimize_sparse_affine_kr_map only uses neighbors j < k and still adds the diagonal term explicitly.
@dafeda
Add gstools dependency for notebooks
a9a14be 
gstools is imported by the notebooks and should be available in the default environment.
@dafeda dafeda force-pushed the no-self-loops branch 3 times, most recently from 30ba99b to a564c5a Compare May 4, 2026 13:56
@dafeda

dafeda commented May 5, 2026

Copy link
Copy Markdown
Collaborator Author

I see the function does not seem to be used or tested anywhere, but might be useful in the future?

fit_precision_cholesky_approximate is used in ert. But yeah, perhaps we should add a test...

Oh, I see. Then there should definitely be tests for it...

I have added a property test.

The commit where I exclude self-loops should not change behavior.
I wrote this in the commit message body:

"This does not change current fitting behavior because optimize_sparse_affine_kr_map only uses neighbors j < k and still adds the diagonal term explicitly."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsevj larsevj larsevj approved these changes

Assignees

@dafeda dafeda

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dafeda @larsevj