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.
>>
>> 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 ;-).
Thomas
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
