On 9/17/13 2:07 PM, Thomas Neidhart wrote:
> On 09/17/2013 04:22 PM, Evan Ward wrote:
>> Hi Luc,
>>
>> On Mon 16 Sep 2013 12:04:21 PM EDT, Luc Maisonobe wrote:
>>> Hello,
>>>
>>> I have started (at last) to work on the fluent API for ODE integrators.
>>> It seems to be well suited for the needs, and would allow a better
>>> separation of the integrator "engine" and the integrator "state". The
>>> engine will hold all the coefficients for the linear combinations within
>>> the integration steps and some general settings like min and max step
>>> sizes, or absolute and relative error thresholds for adaptive stepsize
>>> integrators. These data don't change during one integration run, and
>>> user may want to share them among several successive or parallel
>>> integration runs. The state will hold more transient values for numerous
>>> internal variables, like step size, start of step, pending discrete
>>> events, number of evaluations ... These data are updated continuously
>>> during an integration run and can certainly not be shared.
>>>
>>> Users will only see and configure the engine. Each time they call the
>>> integrate method on an engine, it will create on the fly a state
>>> dedicated for this call and everything within the integrator will
>>> reference this state instance. Users may call the integrate method
>>> several time to run them in parallel in different threads. As each call
>>> will have its own state instance, and as the engine itself which is
>>> shared by all states is immutable, everything should work without problem.
This sounds reasonable to me. Two things you might want to think about:
0) It might make sense to actually split config / state data into
three parts instead of two:
a) problem instance definition (the coefficients, for example go here)
b) engine configuration (step size, other engine config)
c) algorithm state (what you call integrator state)
1) Be careful with mutable == state. Item c) above might logically
include some runtime immutable stuff.
>>>
>>> These were the good news.
>>>
>>> The bad news are that it is difficult to separate the state from the
>>> engine as I have mixed everything right from the top level interface,
>>> which in this case is ODEIntegrator. The interface defines methods like
>>> getCurrentStepStart (which should refer to state) but also
>>> addStepHandler/addEventHandler (which should refer to engine). The next
>>> level interface (FirstOrederIntegrator) adds an integrate method which
>>> should belong to engine. The AbstractIntegrator class implements many of
>>> these methods and adds new protected methods which can be used by
>>> specific integrators.
>>>
>>> My current design is to have an internal IntegratorState class defined
>>> within AbstractIntegrator and providing all the state related fields and
>>> methods. For now, I don't think there is no need here for interfaces or
>>> abstract classes, the State is a simple thing. I may introduce a small
>>> hierarchy later on when I delve into the specific integrators, though.
>>> The integrate method in AbstractIntegrator would simply be something like:
>>>
>>> public void integrate(ExpandableStatefulODE equations, double t) {
>>> integrate(new IntegratorState(this), equations, t);
>>> }
>>>
>>> protected abstract integrate(IntegratorState state,
>>> ExpandableStatefulODE equations,
>>> double t);
>>>
>>> The protected integrate method with the state argument would be the same
>>> as the current integrate methods, just storing and updating variables in
>>> the State instance rather than in the integrator instance as done today.
>>>
>>> This should handle the engine part as desired, with a state instance
>>> created on the fly for each integration run.
>> +1 I like it. I think it will make the integrators easier to use and
>> more useful. :)
> I like the state idea too, a pattern I use a lot myself.
>
>>> Most of the IntegratorState class would be copied from the current
>>> AbstractIntegrator class, where many state related stuff is managed now.
>>> The problem is that many of these fields are protected fields
>>> (stepStart, stepSize, isLastStep, ...) and many methods are protected or
>>> even public methods (initIntegration, computeDerivatives). Of course,
>>> there are also the public methods inherited from the top level interface
>>> like getCurrentStepStart. I cannot simply remove the public method from
>>> the interface, nor remove the protected fields and methods from the
>>> AbstractIntegrator class, this would break compatibility...
>>>
>>> Most of these methods were never intended to be in the public API of the
>>> library, but due to Java limitations with cross packages visibility,
>>> some were put public. In the same spirit, the protected methods are not
>>> expected to be in the component API as it is not expected that people
>>> will provide their own implementations of ODE integrators: all the
>>> implementations and all the callers of the methods are within [math].
>>>
>>> So how can I handle these methods and fields? At least, I will deprecate
>>> them. I will also change all our own implementations to use the new
>>> IntegratorState class to store and update all these transient variables,
>>> so from [math] point of view, the deprecated protected methods and
>>> fields will not be used anymore. What should I do with the remnants and
>>> unused fields? As long as we are in the 3.X series, they should remain
>>> here, but how should they behave? Lets take one example: there is one
>>> method, intended to be called only by the specific integrators
>>> implementations:
>>>
>>> protected double acceptStep(AbstractStepInterpolator,
>>> double[], double[], double);
>>>
>>> It is called by AdamsBashforthIntegrator, AdamsMoultonIntegrator,
>>> EmbeddedRungeKuttaIntegrator, GraggBulirschStoerIntegrator and
>>> RungeKuttaIntegrator with lines like:
>>>
>>> stepStart = acceptStep(interpolator, y1, yDot1, t);
>>>
>>> All these calls will be changed to something like:
>>>
>>> state.setStepStart(state.acceptStep(interpolator, y1, yDot1, t));
>>>
>>> This means only the acceptStep of the new IntegratorState class will be
>>> called (and it will almost be a copy of the former method, moved to a
>>> new class). What should the acceptStep method in the AbstractIntegrator
>>> class do? It will not be called anymore by ourselves, and will not serve
>>> any purpose anymore. Still I cannot remove it.
>>>
>>> I certainly do not want to duplicate the entire package. We have seen
>>> users are lost when we do this and get questions on the list due to the
>>> confusion and the fact people either don't see the new package or see
>>> several packages and don't know which one to use.
>>>
>>> I have found one solution, but it is really ugly. I could temporarily
>>> add the IntegratorState instance as a field in AbstractIntegrator and
>>> have the integrator delegate to the State, as follows:
>>>
>>> public abstract class AbstractIntegrator
>>> implements FirstOrderIntegrator {
>>>
>>> /** Current step start time.
>>> * @deprecated as of 3.3 replaced by IntegratorState#stepStart
>>> */
>>> protected double stepStart;
>>>
>>> /** Last state instance.
>>> * Corresponds to last call to integrate().
>>> * @since 3.3
>>> * @deprecated temporary hack introduced in 3.3,
>>> to be removed in 4.0
>>> */
>>> @Deprecated
>>> private IntegratorState lastAllocatedState;
>>>
>>> /** Container for transient integration data.
>>> * @since 3.3
>>> */
>>> protected class IntegratorState {
>>>
>>> /** Current step start time. */
>>> private double stepStart;
>>>
>>> /** Get current step start time.
>>> * @return current step start time
>>> */
>>> public double getCurrentStepStart() {
>>> return stepStart;
>>> }
>>>
>>> /** Set current step start time.
>>> * @param current step start time
>>> */
>>> public void setCurrentStepStart(double stepStart) {
>>> this.stepStart = stepStart;
>>> // TODO: to be removed for 4.0
>>> AbstractIntegrator.this.stepStart = stepStart;
>>> }
>>>
>>> }
>>>
>>> /** {@inheritDoc}
>>> * @deprecated as of 3.3 replaced by
>>> * IntegratorState#getCurrentStepStart()
>>> */
>>> public double getCurrentStepStart() {
>>> return lastAllocatedState.getCurrentStepStart();
>>> }
>>>
>>> public void integrate(ExpandableStatefulODE equations, double t) {
>>>
>>> final IntegratorState localState = new IntegratorState(this);
>>>
>>> // TODO: to be removed for 4.0
>>> lastAllocatedState = localState;
>>>
>>> integrate(localState, equations, t);
>>>
>>> }
>>>
>>> protected abstract integrate(IntegratorState state,
>>> ExpandableStatefulODE equations,
>>> double t);
>>>
>>> }
>>>
>>> Here, we preserve the stepStart protected field, it will be updated
>>> automatically by the new code that uses IntegratorState.setStepStart,
>>> users may read it from dereived classes. We also preserve the
>>> getCurrentStepStart method, which delegates to the last allocated state.
>>> What will *not* work however is when people who would have developed
>>> their own integrator would try to update the field from their derived
>>> class. They will be able to do it, but won't have the desired effect
>>> since [math] doesn't use it anymore. Yes, I know, it was bad design
>>> right from the beginning to have a protected field, my bad.
>>>
>>> Of course, this would mean that each new integrate run would override
>>> the lastAllocatedState instance, but this is similar to the current
>>> behaviour. If people attempt to call integrate several time in parallel,
>>> lots of contingency problems will appear right now, so we don't
>>> introduce a new problem here. The field would be removed from
>>> AbstractIntegrator in 4.0 when the methods will be removed and all
>>> access will be limited at IntegratorState level.
>>>
>>> What do you think?
>> IMHO just make the change for 4.0. I think it would be easier for the
>> users to only have to learn the intricacies of one new API. Especially
>> since we know the deprecated 3.X API will be confusing and not
>> completely backward compatible. Just my $0.02.
> I agree with Evan, your proposed solution may work technically to
> preserve binary compatibility, but will break client code anyway.
>
> Such a change would only be acceptable for a major release imho. Otoh we
> have already quite some content for a 3.3 release. Let's try to release
> this in, let's say 1 month, and then work on a 4.0 release, cleaning up
> all the API drawbacks/flaws we have accumulated in the last years.
>
> As you already said, several users raised their concern about all the
> deprecated stuff in commons-math, I think we should not create more
> headaches (especially for ourselves ;-).
Big +1 here. Lets get 3.3 out and get this right in 4.0 without
trying to preserve compat.
Phil
>
> Thomas
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
