ArcetriAdaptiveOptics / arte

Arcetri Random sTuff collEction
MIT License
2 stars 0 forks source link

ShapeFitter fixes #22

Closed lbusoni closed 3 years ago

lbusoni commented 3 years ago

pep8, unused imports. Test of correlation method doesn't pass. The return values are probaly swapped

lbusoni commented 3 years ago

@imapeartree : I fixed ShapeFitter. While I was simplifying the assert syntax in the tests I noticed that the test for the correlation method is not passing because the return values are swapped in order. The tests as they were written before are passing only because the check is somehow wrong.

I created the branch fix_shape_fitter: if you check test_shape_fitter.py you'll see that test_correlation() doesn't pass and you'll see that test_correlation_passes_but_it_is_wrong() passes but it shouldn't!

You should check if I understood the tests correctly and fix the ShapeFitter.fit() in the method="" case.

Every time you commit into this branch, this pull-request will run the checks again; only when the tests are ok you can merge the pull request that I originated

lbusoni commented 3 years ago

I also prefer to have parameters() instead of params() (and I already modified it!)

I think it is also preferrable to have

ShapeFitter.fit_circle_with_correlation()
ShapeFitter.fit_circle_with_ransac()
ShapeFitter.fit_something_with_algo()

instead of a generic fit() with arguments

lbusoni commented 3 years ago

Also, the keyword usecanny=True in fit() is not used. It must be removed

lbusoni commented 3 years ago

Finally, the name and arguments of the public methods must be carefully chosen and must stay fixed forever (as they will be used outside of this library). < Another argument against a too-much-generic fit() method >

Please, write the documentation of the public methods.

You can follow code_convention.py or https://numpydoc.readthedocs.io/

A good example is also in von_karman_psd.py

imapeartree commented 3 years ago

Please note the different behavior between the minimization algorithms which swaps cx and cy coordinates in output on fit_ circle_correlation method

lbusoni commented 3 years ago

The swapping was not a swapping: the initial guess of x and y was swapped, so you passed y,x and the methods that you listed are simply not optimizing anything returning the initial guess. This is also why you have an error in the radius when using AnnularMask: the guess is done computing image moments that results in a larger radius. It is actually quite complex to understand if the minimization is run at all; we are probably misusing optimize.minimize in this application and my guess is that this relates to the lack of gradient computation.

Do we need to have all these methods? Can't we simply use the 3 that are working?

imapeartree commented 3 years ago

(missing from the commit) Done Split the fitting function adding cicle fitting and anular fitting