solve_trig sometimes returns an incorrect root #123
Conversation
…es to _solve_trig to return all 3 real roots instead of the 1st one from the trigonometric solution, added static _pick_root which picks the correct root to use, changes to calc_xc which now uses _pick_root to pick the correct root.
|
Thanks, I'll try to have a look a this later today or this week. Just for any potential future contributions; it's easier for me if you create a feature branch on your work (instead of using master) and then if you make a PR tick the box 'Allow edits from maintainers'. |
|
Sounds good, I'll create a separate branch in my fork and go from there! |
|
By the way, as far as I see now, there appear to be no further issues. If I come across anything, I will try to fix and send to you. |
|
Ok, thanks. |
|
Hi, sorry for the delay, we were moving our lab to a new site this month and things have been super busy ... but getting back into things now. Looking at the test, we have one failure in test_fitting_brightfield_DE, the DE optimiser returns a slightly different value (8197140.50771191 != 8191888.515532021) when optimising the cell. Since DE is quite stochastic iirc, this doesn't sound like a major problem. Let me know what you think! |
|
No worries, your work, your rules! High code quality is definitively something to fight hard for! :) |
solve_trig by default returns the first root (k=0) of the trigonometric solution, which is sometimes not the correct root. This leads to errors in calculation of x_c which propagates to the radial distribution calculation and cell reconstrution, particularily when the cells become vertical.
Fixed by returning all 3 roots (k=0,1,2) from solve_trig, and by adding a static method to evaluate the r^2 polynomial expression at all 3 roots and pick the root that minimizes r^2. A purely analytic solution is also possible based on the sign of the second deriviative, but in practice this was fewer lines of code, and that would have the issue of inflexion points.
To reproduce a test case, initialise the parameters and bitmap provided in #119 and reconstruct over the binary. Current code produces the incorrect reconstruction (bottom), fix produces the correct reconstruction (top). Same holds for fluorescent data, not shown here for brevity. (Sorry for small image size of bitmap, they're the same pixel size as the mask which generated them)
fixes #122