commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gilles <gil...@harfang.homelinux.org>
Subject Re: [math] leastsquares refactoring
Date Sun, 16 Feb 2014 17:28:55 GMT
Hi Luc.

>>>> [...]
>>>> 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.
>

Hmm, please take into account that "fitting.leastsquares" was meant as 
a
sort of proof-of-concept for a new API (mainly, the fluent API which 
you
proposed) for _all_ optimizers. If 3.3 is to be released with the 
current
trunk (modulo your pending changes in "fitting.leastsquares"), users 
won't
have any transition period for all the other optimizers which are to be
removed in 4.0 (unless we keep the old cruft for another generation, 
which
would be contrary to your stated goal of clean-up).
Hence either a 3.4 release is necessary to complete the API change, or 
3.3
must be delayed until all the optimizers are modified similarly to 
Evan's
proposal. But in this latter case, the change would happen even before
actually allowing users to provide feedback on the new API (by testing 
it
on their least-squares use-cases)...


Regards,
Gilles


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


Mime
View raw message