Skip to content

graph with inhom compartments#779

Merged
jnsbck merged 10 commits intojaxleyverse:mainfrom
NicolasRR:main
Jan 30, 2026
Merged

graph with inhom compartments#779
jnsbck merged 10 commits intojaxleyverse:mainfrom
NicolasRR:main

Conversation

@NicolasRR
Copy link
Contributor

Handles building a cell from a graph with inhomogeneous branches, i.e. different number of compartments.

@NicolasRR NicolasRR closed this Jan 12, 2026
@NicolasRR NicolasRR reopened this Jan 12, 2026
Copy link
Contributor

@jnsbck jnsbck left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution. I think this is nice to have and I am fine to merge this in principle. Please have a look at the comments I left and make sure all the checks and tests pass.

counts_by_branch = cell_groups.groupby("branch_index").size().sort_index()

for _, ncomps in counts_by_branch.items():
branches.append(Branch([comp for _ in range(int(ncomps))]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add a check to see if all ncomps are the same. If yes, then instantiate the branches as before, i.e. [branch] * num_branches. This is much faster since only one Branch instance needs to be created, rather than many.

@NicolasRR NicolasRR requested a review from jnsbck January 13, 2026 14:48
Copy link
Contributor

@jnsbck jnsbck left a comment

Choose a reason for hiding this comment

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

This is getting there! I left a few minor comments to be addressed. Also please rebase (#777 was just merged) and update the changelog. If all checks pass I am happy to merge this :)


def node_match(a, b):
for key in a.keys():
try:
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use an isinstance rather than a try except if possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also which attributes is the exception for?

In addition, you might wanna use np.isclose rather than != on floats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exception is raised for xyzr which is an array
I also fixed the float comparison in the test

@NicolasRR NicolasRR requested a review from jnsbck January 14, 2026 14:46
Copy link
Contributor

@jnsbck jnsbck left a comment

Choose a reason for hiding this comment

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

I still had some questions. Sorry for the back and forth. See comments below. Also make sure to resolve the conflicts.

@NicolasRR NicolasRR requested a review from jnsbck January 19, 2026 15:27
Copy link
Contributor

@jnsbck jnsbck left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for iterating on this. Many thanks for contributing :)

Please make sure to run isort and black. If the checks pass, I can merge this.

@jnsbck jnsbck merged commit fa4fd52 into jaxleyverse:main Jan 30, 2026
1 of 2 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