commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luc Maisonobe <...@spaceroots.org>
Subject Re: [math] leastsquares refactoring
Date Sun, 16 Feb 2014 12:51:08 GMT
Le 14/02/2014 16:33, Evan Ward a écrit :
> 
> On 02/13/2014 12:19 PM, Gilles wrote:
>> Hello Luc.
>>
>> On Thu, 13 Feb 2014 16:10:36 +0100, Luc Maisonobe wrote:
>>> Hi Gilles,
>>>
>>> Le 08/02/2014 22:16, Gilles a écrit :
>>>
>>>>> Other issues to be discussed:
>>>>>
>>>>>  * [MATH-1008]
>>>>>
>>>>>    a new package has been created o.a.c.m.fitting.leastsquares which
>>>>>    contains some interface for the new fluent API but they are also
>>>>>    used elsewhere so I guess it would be more logical to put them in
a
>>>>>    more general place, e.g. WithMaxEvaluations should be moved to
>>>>>    o.a.c.m.optim. WDYT?
>>>>
>>>> Strictly speaking, these have potential use outside the "o.a.c.m.optim"
>>>> package.
>>>>
>>>> Also, further improvements were postponed since the alternate proposal
>>>> looked quite promising:
>>>>   https://issues.apache.org/jira/browse/MATH-1026
>>>>
>>>
>>> I had a look at Evan's poposal in MATH-1026. I really like it!
>>>
>>> Do you think we could replace our current lestsquares (which has never
>>> been released yet), with this new one and finish it?
>>
>> In principle yes (see my first comment on the JIRA page). In practice,
>> I'm
>> not sure.
>>
>>> Concerning the last
>>> comment by Evan in the JIRA system, I think they could be answered
>>> quickly. Also the questions about caching and lazy evaluations are
>>> implementation details that could be changed later on, even after 3.3
>>> release.
>>
>> My memory about all the discussions fades away. IIRC, the main problem
>> with the proposed code is that it provided supposed enhancements to the
>> functionality before showing that it could faithfully reproduce the
>> existing functionality (albeit with another API).
>> Because I could not be sure of the equivalence and did not have time to
>> make changes and checks, I did not want to take the responsibility for
>> committing the code in that uncertain state.
>> Maybe it's fine; I just don't know.
> 
> I would just like to add that I made a significant effort to ensure the
> API update is in a separate commit from all the other improvements.
> Commit 7e9a15e [1] contains the API switch. As with any API change it
> involved updating the test cases, so I realize there is a lot of code to
> read to check for correctness. I did factor common test code into
> reusable methods, but IMHO this should make it easier to review since
> there is only one opportunity for a typo instead of dozens.
> 
> All the following commits in the pull request contain the functionality
> improvements and code quality improvements (as well as moving to a
> different package and test style changes).
> 
> Please let me know if you have any questions about the code or about why
> I made a particular change.
> 
> Best Regards,
> Evan
> 
> 
> [1]
> https://github.com/wardev/commons-math/commit/7e9a15efb535b91afad9d8275eb2864ea2295ab4
> 
>> Also, please take into account that, additionally to the API change for
>> the code now in "fitting.leastsquares", there has been some "little"
>> improvements in the code itself (e.g. compare the implementations of
>> "LevenbergMarquardtOptimizer" in "optim" vs "fitting.leastsquares").
>> Before trashing this new code, it would be good to make sure that those
>> (tested) improvements have been retained in Evan's code.

Looking at the code, it really seems Evan started from your
fitting.leastsquares (in particular, the InternalData internal class
introduced in your class is also in Evan's class).

>> To sum up, what is in "fitting.leastsquares" should be the reference
>> code, in the sense that post-release 3.3, _nothing_ from the deprecated
>> "optimization" and "optim.nonlinear.vector" should make its way to code
>> for 4.0.
>> Hence, I'd favour keeping the current "fitting.leastsquares" until we are
>> positively sure that everything there was indeed ported to Evan's code
>> (whenever applicable).
>> We could put both in an "experimental" sub-package; that would not delay
>> a 3.3 release, while leaving room for non-compatible changes in a 3.4
>> release.

I don't like this. We has two versions of LevenbergMarquardt in previous
version, and by introducing both fitting.leastsquares and
fitting.leastsquares2 would mean we have four implementations! I really
think fitting.leastsquares should retain only one, and as the one from
Evan was started from the one you did, it is the more advanced. So I
would select this one.

I also am quite reluctant to a 3.4 release. We should really move
forward and clean up old stuff, which can be done only by pushing a 4.0
release after 3.3.

So if nobody complains, I will start updating leastsquares with the code
from MATH-1026, perhaps starting directly from the commits in the git
repository if possible. As I use git-svn as my tool of choice, this
should be achievable.

best regards,
Luc

>>
>>
>> Best regards,
>> Gilles
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 
> 
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message