Skip to content

Conversation

@matham
Copy link
Contributor

@matham matham commented Jun 7, 2025

Description

This PR depends on #542.

Added a file under benchmarks (filter_debug.py) that given an input folder containing a list of tiff, it loads it using dask and does full 2d and 3d filtering, cell detection and cluster splitting. For each filtering step it outputs the filtered image so you can see what the step produces and tune parameters.

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?

Because it's hard to tune all these parameters when you only have the final output (candidate cells), which takes a bit to run and you have to guess at how these input parameters actually affect the outcome. There's already something like this for saving the output of the 3d filtering step, but having the 2d step output in particular is helpful.

This should also be helpful for generating images of the output for docs to show what each of the algorithm steps are doing.

You can see how I used this in #532 and in #545.

What does this PR do?

Add the filter_debug.py file under benchmarks. If we had an examples folder I would have used that.

I also had to add copying settings during splitting because we change the input settings during splitting. Previously it wasn't a problem because it's each cluster was split in its own process so each settings was copied anyway. But now I was doing it all in one thread so the original settings instance was modified for future clusters.

It would be nice to be able to visualize all these debug outputs directly in napari, but that would require a lot more work and possibly not worth the complexity. Although I do wonder how most users would approach this tuning stage without being able to see how the parameters change the output. And I suspect something like this would be invaluable when tuning parameters for new samples.

References

#532 and in #545.

How has this PR been tested?

I tested it with data.

Is this a breaking change?

No.

Does this PR require an update to the documentation?

No.

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality (unit & integration)
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copy link
Member

@alessandrofelder alessandrofelder left a comment

Choose a reason for hiding this comment

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

Amazing, thanks @matham

This should also be helpful for generating images of the output for docs to show what each of the algorithm steps are doing.

SO helpful 🤩

It would be nice to be able to visualize all these debug outputs directly in napari, but that would require a lot more work and possibly not worth the complexity. Although I do wonder how most users would approach this tuning stage without being able to see how the parameters change the output. And I suspect something like this would be invaluable when tuning parameters for new samples.

Completely agree with all of this, and it's probably something we haven't needed for most rabies-tracing type applications (most defaults usually "good enough") but becomes more important as cell candidate numbers grow. Maybe like a "preview" button that outputs these images for the current slice in napari? This should be a separate discussion, and better docs takes priority over this feature, I think.

My wishlist on this PR

  • a tiny feature request for an additional output (cell candidates as an xml)
  • rebase onto latest #542
    Then I think we can accept this.

- struct_type_split: Same as struct_type except we put a sphere with value 1
centered on the structures that were split into cells instead of `2`.

There's also a `structures.csv` output that lists all the detected structures,
Copy link
Member

Choose a reason for hiding this comment

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

Could we also save cell candidates in brainglobe format?
That would be incredibly helpful for documentation screenshots, and comparing versions of the code visually?

@matham matham force-pushed the vis_filters branch 2 times, most recently from b70b179 to 5ac12ed Compare June 10, 2025 21:22
@matham
Copy link
Contributor Author

matham commented Jun 10, 2025

I rebased/squashed and added the xml output.

There's a bit of idiosyncrasy I think in how the cells are typed and loaded in napari. The options seems to be either artifact/unknown (what's the difference? and why when saving does artifact get turned into unknown? it's not very clear from the docs) and cell. But what if I wanted to save them as different types? E.g. here I'd have loved to be able to save them as 4 types: cell, cell after splitting, struct to be split, and struct too large to be split. But I had to fit it into those 2 types so I just ended up with 4 files.

Related, typically when saving a layer from napari, a layer of points, the xml files is named based on the layer name, which can be anything. But when I open the xml in napari, the original filename is gone and it's named as cells and not cells. This makes it hard to keep track of what layer came from which file. Plus, could there not be more than 2 cell types?

@adamltyson
Copy link
Member

Basically, the current xml format for saving cells is a historical artefact. I've been wanting to come up with a better format for a while to address these issues (and more), but not got round to it yet. In principle it shouldn't be too complex to come up with a new, much more flexible format.

@matham
Copy link
Contributor Author

matham commented Jun 17, 2025

I rebased on #542.

@alessandrofelder
Copy link
Member

This might need another rebase @matham (I think because I squashed #542, which is our usual practice) but this is good to go too! Thanks so much 🎉

@alessandrofelder alessandrofelder self-requested a review June 23, 2025 16:38
@matham
Copy link
Contributor Author

matham commented Jun 23, 2025

Rebased!

@alessandrofelder alessandrofelder merged commit 25af82e into brainglobe:main Jun 24, 2025
18 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.

3 participants