Option to use full polynomial in ML linesearch #487
Conversation
There was a problem hiding this comment.
Can be merged. Not sure why the old way (internal quad_approx switch) is not used instead of the suggested duplicate code. See https://github.com/ptycho/legacy-ptycho/blob/2fade96aaf773a11c3aae318868ac711a0a5ff6c/lib/core_object_modes.py#L3494
| self.B = B | ||
|
|
||
| return B | ||
|
|
||
| def poly_line_all_coeffs(self, ob_h, pr_h): |
There was a problem hiding this comment.
Wouldn't it make sense to have a single method with an internal switch based on 'quadratic' or 'all'? It feels like there is a lot of repetition between the two...
There was a problem hiding this comment.
Yes there is a lot of code duplication, but I believe @daurer preferred it this way as there's no risk of breakage...
|
@pierrethibault simply because I don't have access to ptycho-legacy so I didn't know there was an old way. |
|
As discussed with @daurer and @bjoernenders, we can merge this, and keep in mind possible improvements to reduce code duplication, and possibly to allow other truncation levels since the line-search for the gaussian model is an order 8 polynomial and the Taylor expansion of the Poisson and Euclidean models is infinite. |
Resurrecting #392 as GitHub broke the original PR.
It has been observed that ML shows spikes in the likelihood error, this appears to be due to the use of an approximate quadratic polynomial in the linesearch instead of the full octic polynomial as described in the ML paper. This PR adds an option
all_line_coeffsto enable the full octic polynomial at at cost of about 60% more computation in the coefficient calculation stage at each iteration.Caveats:
np.rootsonly supportsnp.doublenotnp.longdouble(may need to write our own roots function). This is not an issue for the moonflower test dataset as the full precision of long double is not required there. However, it probably will be on some real datasets.