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

Robustspotdetection #34

Merged
merged 52 commits into from Apr 23, 2020
Merged

Robustspotdetection #34

merged 52 commits into from Apr 23, 2020

Conversation

jennyfolkesson
Copy link
Contributor

I believe this should be stable now, but @bryantChhun should have a final say before this is merged. I wanted to start the code review at least.

One question - it looks like a lot of images are displayed now as you're running, and that makes the run pretty slow, plus you have all these plots popping up. What about changing this to write only?

@mattersoflight
Copy link
Member

mattersoflight commented Apr 8, 2020

I suggest @smguo merge his recent changes that improve the estimation of OD into this branch, and then we test it against March 25 and Apr 4 datasets.

@jennyfolkesson these plots are to catch edge cases as the pipeline is developed, but they are all invoked if debug is enabled. If there are any plots that are outside if debug: flag, we should move them there.

@bryantChhun
Copy link
Contributor

There is a minor issue with the OD calculation -- both the cropped image and the background need to be on range 0-1, float64, not inverted

I will submit a PR to robostspotdetection

@bryantChhun
Copy link
Contributor

I'll work on merging this into master. There was a bit of module-movement, so I'll need to refactor a little, then retest across test data using interp/fit/well.

Will create a new branch for those changes and submit a PR here.

@jennyfolkesson
Copy link
Contributor Author

jennyfolkesson commented Apr 21, 2020 via email

bryantChhun and others added 4 commits April 21, 2020 15:01
…into robustspotdetection

fixed bug in well_wf.py that prevented method='well_crop' from outputting debug images

# Conflicts:
#	array_analyzer/extract/image_parser.py
clearer variable naming
Copy link
Contributor

@bryantChhun bryantChhun left a comment

Choose a reason for hiding this comment

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

I'm ok to merge this PR as is

output_name,
from_source=True,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like the plots for the background images have been removed and are not present in the "debug_images" module. Plotting background images were exactly what told me there was an issue with scaling and bitdepth. Will this become a test instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @bryantChhun. I think it would be better to keep some of the useful debug plots or move them to "debug_images" in case we need to look at them in the future. We can speed up the processing by running in regular mode instead of debug mode if the speed is the concern here.

Copy link
Member

@mattersoflight mattersoflight left a comment

Choose a reason for hiding this comment

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

I tested the branch with last two datasets (blinded flu plate and covid concentration test plate) for accuracy of spot detection and optical density estimation. They both look great. Ready to merge with debug plots for background added back to the pipeline.

Copy link
Contributor

@smguo smguo left a comment

Choose a reason for hiding this comment

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

@jennyfolkesson The PR looks great. I only have some minor suggestions. Feel free to merge when you think it's ready.

run_array_analyzer.py Outdated Show resolved Hide resolved
array_analyzer/workflows/registration_workflow.py Outdated Show resolved Hide resolved
dtype=object,
)

nbr_grid_rows, nbr_grid_cols = props_array.shape
Copy link
Contributor

Choose a reason for hiding this comment

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

Are nbr_grid_rows, nbr_grid_cols same as params['rows'], params['columns']?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, look at that! I didn't pay much attention to params I guess... :-)

output_dir=args.output,
method=args.method,
debug=args.debug,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

output_name,
from_source=True,
)

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @bryantChhun. I think it would be better to keep some of the useful debug plots or move them to "debug_images" in case we need to look at them in the future. We can speed up the processing by running in regular mode instead of debug mode if the speed is the concern here.


im_norm = cv.GaussianBlur(im, (blur_sigma, blur_sigma), 0)
# Black top hat filter, turns spots bright
im_norm = black_tophat(im_norm, size=(tophat_size, tophat_size))
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious about the purpose of adding black tophat. Is blob detector better at detecting bright v.s. dark spots?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the blob detector just needs a flag to know if the spots are bright or dark.
The black tophat is for detecting dark spots whereas a regular tophat would detect bright spots.

trans_coords = trans_coords[0].astype(np.float32)
# Find nearest spots
ret, results, neighbors, dist = knn.findNearest(trans_coords, 1)
if remove_outlier:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the the outlier (fiducial that is not detected) will be removed starting at the first iteration. I'm wondering if it's possible that the wrong fiducial will be removed if there happens to be a spot close to the missing fiducial in the initial grid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With enough stochasticity it doesn't seem to be a problem. All the other spots need to be in agreement with the grid so if there's one spot that's not quite on the grid that will still be the one excluded would be my guess.

Copy link
Contributor

@smguo smguo Apr 23, 2020

Choose a reason for hiding this comment

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

I see. I guess the particles with the missing fiducial distance removed will have higher weights than the wrong solution, so these particles will eventually dominate through resampling. Very cool!

ret, results, neighbors, dist = knn.findNearest(trans_coords, 1)
if remove_outlier:
# Remove worst fitted spot
remove_spot = np.where(np.squeeze(dist) != np.amax(dist))[0]
Copy link
Contributor

@smguo smguo Apr 22, 2020

Choose a reason for hiding this comment

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

Is this line to get the Boolean indices of the distances to keep? if so wouldn't dist_ids = (dist != np.amax(dist)) do the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks (what was I thinking...).

if remove_outlier:
# Remove worst fitted spot
remove_spot = np.where(np.squeeze(dist) != np.amax(dist))[0]
dist = dist[remove_spot]
Copy link
Contributor

@smguo smguo Apr 22, 2020

Choose a reason for hiding this comment

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

Is remove_spot actually indices of the distances to keep? Maybe change the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, that was a bad name. I removed that variable altogether.

@jennyfolkesson jennyfolkesson merged commit a7700dc into master Apr 23, 2020
Copy link
Contributor

@bryantChhun bryantChhun left a comment

Choose a reason for hiding this comment

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

...

@jennyfolkesson jennyfolkesson deleted the robustspotdetection branch April 23, 2020 17:27
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.

None yet

4 participants