AllenInstitute / em_stitch

EM microscopy lens correction
BSD 2-Clause "Simplified" License
4 stars 3 forks source link

solve using resolvedtiles and matches, some optimization #43

Closed RussTorres closed 4 years ago

RussTorres commented 4 years ago

Adds a new function to use renderapi.resolvedtiles.ResolvedTiles and matches in the render "list of dict" format to run thelens correction. As I was trying to do this interactively in a notebook, this PR also has some optimizations.

My environment was likely not optimal for this, but for my use case the changes in this PR reduced the time to assemble and solve from >4mins to ~10s. Performance changes made are listed below:

Most of these modifications are on by default, but should be exposed via kwargs to em_stitch.lens_correction.mesh_and_solve_transform._solve_resolved and em_stitch.lens_correction.mesh_and_solve_transform.create_A

djkapner commented 4 years ago

It has not been the norm in this repo to include docstrings, but,since the updates are pretty infrequent, it might help future dives into the code. I imagine they might have helped you out on this refactor if they had been there. I'd advocate for numpy style docstrings on new functions. Similarly, I did not do a great job of including unit tests originally, but, they'd probably be a good addition for some of these new smaller functions. It seems like you're passing integration tests without modifying any test code. So, I wonder what's the intent for keeping the legacy behavior? We could add a unit test comparison for new vs. legacy and then drop that test an the legacy code in the future. Unless I am missing some legacy use-case. We can discuss here, but maybe I'll just ask you offline, what's going on on the microscopes that there needs to be parameter sweeps now? I'm wondering if some flaw in this repo was exposed, or there is some new phenomenon in the scopes. These are just suggestions. If you don't want to invest the time, I won't hold it up.

RussTorres commented 4 years ago

Most of the legacy options should be able to be removed without changing anything, but I was not sure if there were reasons to keep them around. It would be nice to see the new behavior in the wild before we throw out some of the options that could have different outputs, like the non-bruteforce simplex finding and the bounding-box based density smoothing (which could include borders a little bit differently than the previous function).

I agree that docstrings would have helped -- I will add them for functions this PR touches, but created #44 to track further work.

I should also be able to add some tests for new functions.

codecov-io commented 4 years ago

Codecov Report

Merging #43 into develop will decrease coverage by 3.05%. The diff coverage is 90.90%.

@@             Coverage Diff             @@
##           develop      #43      +/-   ##
===========================================
- Coverage    93.92%   90.87%   -3.06%     
===========================================
  Files           10       10              
  Lines          906      964      +58     
===========================================
+ Hits           851      876      +25     
- Misses          55       88      +33