search

spacetx / starfish

starfish: unified pipelines for image-based transcriptomics
https://spacetx-starfish.readthedocs.io/en/latest/
MIT License
221 stars 67 forks source link

Fix coords assignment in CombineAdjustFeatures.run #1965

Closed sgratiy closed 2 years ago

sgratiy commented 2 years ago

This PR fixes a bug in with the CombineAdjustFeatures.run that was flipping round and channel coordinates in the resulting DecodedIntensityTable compare to the input intensities.

The fix: Use explicit parameter calls when building table coordinates Correct shape definition for the IntensityTable docstring

This notebook shows demonstrates the intensity table coordinates before and after the fix.

njmei commented 2 years ago

@sgratiy FYI, there was a repo configuration setting that was not running the CI for external first-time contributors. I've been told that that issue is now fixed so if you do a force push or something like that it should now properly run the CI.

sgratiy commented 2 years ago

Hey @berl, @neuromusic could you please review this bugfix.

berl commented 2 years ago

@sgratiy sorry this needs more careful checking of downstream impacts. Can you verify that switching the dimensions in the DecodedIntensityTable doesn't impact anything? Even if there's no test coverage, this would likely introduce a breaking change in the output that we use later.

sgratiy commented 2 years ago

@berl I have added the notebook demonstrating the problem in the main description.

berl commented 2 years ago

thanks @sgratiy. For the record, this was indeed a bug in how starfish/core/spots/DetectPixels/combine_adjacent_features.py created a DecodedIntensityTable. This was previously missed because users (and the starfish codebase) never did anything with the IntensityTable part of a DecodedIntensityTable created in this way. I'll revert the revert and we can pretend like this never happened.