Skip to content

Refactor of SWC Reader.#660

Open
jnsbck wants to merge 83 commits into
mainfrom
swc_reader_refactor
Open

Refactor of SWC Reader.#660
jnsbck wants to merge 83 commits into
mainfrom
swc_reader_refactor

Conversation

@jnsbck
Copy link
Copy Markdown
Contributor

@jnsbck jnsbck commented Jun 13, 2025

I refactored the existing SWC reader integrating all the learnings from the past iterations. It is much more concise now. Also runs up to 2x faster.

It should also be much clearer now what is happening. The steps are still the same, i.e. trace branches, compartmentalize, add jaxley metadata, create solve_graph, build module, but there is less util code now.

def nx_to_module(swc_graph: nx.DiGraph, ncomp: int = 1) -> jx.Module:
    comp_graph = compartmentalize(swc_graph, ncomp=ncomp)
    comp_graph = _add_jaxley_meta_data(comp_graph)
    solve_graph = _replace_branchpoints_with_edges(comp_graph)
    module = _build_module(solve_graph)
    return module

def read_swc(fname: str) -> jx.Module:
    swc_graph = swc_to_nx(fname)
    return nx_to_module(swc_graph)

Imported modules appear to match the old ones.

Todos:

  • Add to_graph equiv
  • Integrate into codebase and get tests to pass
  • Address TODO comments
  • Final cleanup

I have not started integrating it yet and kept it seperate for now. Feedback is greatly appreciated.

@jnsbck jnsbck requested a review from michaeldeistler June 13, 2025 15:32
@huangziwei
Copy link
Copy Markdown
Contributor

huangziwei commented Jun 13, 2025

btw, have you ever tried igraph? Switching to it from nx gave me at least 2x~3x boost when computing shortest path distance in my mesh skeletonizer. the documentation is not as good as nx though, but llm should be able to to work out most of the common usages.

@michaeldeistler
Copy link
Copy Markdown
Contributor

We cannot rely on igraph because it is GPL-licensed.

@huangziwei
Copy link
Copy Markdown
Contributor

huangziwei commented Jun 13, 2025

ah, did not know about that. now I need to change the licenses of mines.. :-)

Copy link
Copy Markdown
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Hi @jnsbck,

I started reviewing this but I am really confused---why is all code in tmp.py? Why are zero lines removed? I think there is some stuff missing. Please add it and request another review. Thanks!

Comment thread CHANGELOG.md Outdated
Comment thread jaxley/io/tmp.py Outdated
Comment thread jaxley/io/tmp.py
Comment thread jaxley/io/tmp.py
Comment thread jaxley/io/tmp.py
Comment thread jaxley/io/tmp.py Outdated
Copy link
Copy Markdown
Contributor

@michaeldeistler michaeldeistler left a comment

Choose a reason for hiding this comment

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

Overall this looks awesome, I love how short and fast it is.

A few things:

  1. Let's make sure that graphs are always named explicitly (I see you already do this in later parts of the code of dev.ipynb, but let's also do it consistently in all python modules). I am not a fan of something like:
G = swc_to_nx("../jaxley/tests/swc_files/morph_interrupted_soma.swc")
G = compartmentalize(G, ncomp=1)

and I would prefer

swc_graph = swc_to_nx("../jaxley/tests/swc_files/morph_interrupted_soma.swc")
comp_graph = compartmentalize(swc_graph, ncomp=1)
  1. I actually get much larger speedups than you :) Between 3x and 15x.

  2. You rename quite some functions, e.g. swc_to_nx, compartmentalize. I don't think I have strong opinions, but let's carefully think about naming if we want to change them (again).

  3. Let's have as many code comments on what is happening as possible. I find these parts of the toolbox to be very difficult to follow purely based on code (in part because I do not know nx very well).

  4. Before making things "nice", I would recommend you to test whether all SWC tests pass (if you have not done so yet).

@jnsbck
Copy link
Copy Markdown
Contributor Author

jnsbck commented Jun 16, 2025

Thanks for the feedback! Happy to hear you like it.

  1. Wil do, I agree!
  2. Wow! 15x is insane!
  3. Indeed. I will assimilate them when I merge it into graph.py. I wasnt paying much attention to how they were named originally I have to admit.
  4. Have added a bunch already, but will add more!
  5. For sure! That is why I wanted an initial review, before I take care of tests passing :)

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.

build_compartment_graph doesn't follow min_radius when computing electrical properties Add scaling factor to swc reader

3 participants