Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add decoding functionality to Polaris #36

Merged
merged 71 commits into from
Feb 7, 2023
Merged

Add decoding functionality to Polaris #36

merged 71 commits into from
Feb 7, 2023

Conversation

xuefei-wang
Copy link
Contributor

@xuefei-wang xuefei-wang commented Nov 22, 2022

Add decoding part to the repo. Spot decoding has its own application, and is also wrapped into Polaris.
Tests are also provided.

@elaubsch
Copy link
Member

elaubsch commented Dec 1, 2022

Can you write some tests for the SpotDecoding application and the decoding_functions.py script?

Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Just a couple quick notes on the infrastructure/versioning stuff. As we discussed yesterday, the failures on CI are not related to the changes in this PR itself. The easiest way to fix that would be to merge #40, then merge those changes into this PR. There may be some minor merge conflicts in doing so - I'd be happy to help resolve them if you'd like to team up!

I haven't looked at the code itself yet here - LMK when you think it's at a stage where you'd like a more detailed review!

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@elaubsch elaubsch requested a review from rossbar February 2, 2023 19:04
Copy link
Contributor

@rossbar rossbar left a comment

Choose a reason for hiding this comment

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

Apologies for leaving so many comments - almost all of them can be safely ignored!

I don't see any critical blockers here, aside from doublechecking the dependency specifications (see the comments in requirements.txt and setup.py).

Generally speaking there's a lot of conversion e.g. between dataframes/dicts/arrays etc. that could probably be simplified, but we shouldn't let perfection be the enemy of good. At the very least it was useful for me to walk through the diffs to get a better sense of the operations that go into this workflow.

Two major caveats: I didn't review the notebooks, nor do I have any experience to say anything meaningful about the model construction in decoding_functions.py.

One last thing: running the test suite gives a decent number of RuntimeWarnings, likely due to division by 0 and related floating point arithmetic problems. It's probably worth doublechecking that these are things you expect! You can do so by running the test-suite locally, something like:

# Create a test environment
$ python -m venv dc-spots-testenv
# Make sure working directory is clean
# NOTE: Only run `git clean` if you *don't* have any uncommitted 
# changes that you want to save!
$ git clean -xdf
# Enter clean virtual environment
$ source dc-spots-testenv/bin/activate
# Install dependencies and library locally
$ python -m pip install -r requirements.txt -r test_requirements.txt
$ python -m pip install .
# Run test suite on installed version
$ pytest --pyargs deepcell_spots

deepcell_spots/applications/polaris.py Outdated Show resolved Hide resolved
deepcell_spots/applications/polaris.py Show resolved Hide resolved
deepcell_spots/applications/polaris.py Outdated Show resolved Hide resolved
"""

def __init__(self,
image_type='singplex',
Copy link
Contributor

Choose a reason for hiding this comment

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

tl;dr - this probably doesn't matter, but may break existing code. It's safe to ignore if you don't care about backward compatibility yet!


Technically, adding a kwarg to the front of a signature like this will break any existing code that uses position-based arguments. For example, if there is existing code where a Polaris object was instantiated like:

>>> Polaris(my_seg_model, "cytoplasm")

this change in the signature will break that code, because now the first unnamed argument maps to image_type instead of segmentation_model.

If you don't have a ton of existing code that you're worried about breaking, you can go ahead and ignore this for now! Another solution would be to use keyword-only arguments (i.e. start this signature list with *,) which prevents users from creating Polaris objects with unnamed args. This may still break existing code, but it will fail loudly with a very specific message and a straightforward, future-proof fix.

deepcell_spots/applications/polaris.py Outdated Show resolved Hide resolved
deepcell_spots/decoding_functions.py Outdated Show resolved Hide resolved
deepcell_spots/multiplex.py Show resolved Hide resolved

intensity_d = np.zeros(((extra_pixel_num * 2 + 1) ** 2, num_spots, image_slice.shape[-1]))
d = -1
for dx in np.arange(-extra_pixel_num, extra_pixel_num + 1):
Copy link
Contributor

Choose a reason for hiding this comment

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

There's almost certainly an opportunity to vectorize here, but I'm only breezing over it for now. Another thing to keep in mind for a followup!

requirements.txt Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@xuefei-wang
Copy link
Contributor Author

xuefei-wang commented Feb 6, 2023

I loosely remember that we did something dirty inside Polaris when adding multiplex functionality - for example, this line and related lines should probably be better moved into the detection class. Should we refactor things as well if we want to do things rigorously?

spots_locations = max_cp_array_to_point_list_max(max_proj_images,

@rossbar
Copy link
Contributor

rossbar commented Feb 7, 2023

Should we refactor things as well if we want to do things rigorously?

It might be worth it, but if so I'd advise to leave it for a separate PR. That will make it easier to focus on the specific changes and will prevent blocking this one!

@elaubsch elaubsch merged commit ce7e2de into master Feb 7, 2023
@elaubsch elaubsch deleted the decoding branch February 7, 2023 22:43
@elaubsch elaubsch added the enhancement New feature or request label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants