[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16467597#comment-16467597
]
Gilles commented on MATH-1458:
--
Merged (commit 36553ffab375518a8ce39b25a916f730775435a4 on "master").
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling canIncrement(0) \{ instead of
> canIncrement(nTimes) } to check if the maxCountCallback should be triggered.
> I expect that all uses of the Incrementor actually trigger the
> maxCountCallback when the count has actually exceeded the maximalCount. I
> don't want to get into debugging that class since it also
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16467352#comment-16467352
]
Alex D Herbert commented on MATH-1458:
--
Found 1 checkstyle error for trailing whitespace.
I was careful to never use a code formatting tool and followed the guideline
here:
[CONTRIBUTING|https://github.com/aherbert/commons-math/blob/master/CONTRIBUTING.md]
I ran
{code:java}
mvn clean verify
{code}
but did not know about
{code:java}
mvn site
{code}
It now all builds with no errors.
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16467237#comment-16467237
]
Gilles commented on MATH-1458:
--
Hi.
Thanks for the patch.
Sorry for the nit-pick, but please run
{noformat}
$ mvn site
{noformat}
and check that it did not introduce any CheckStyle errors (the generated site,
with the reports, will be under the {{target/site}} directory).
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling canIncrement(0) \{ instead of
> canIncrement(nTimes) } to check if the maxCountCallback should be triggered.
> I expect that all uses of the
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16467179#comment-16467179
]
Alex D Herbert commented on MATH-1458:
--
Changes submitted via the pull request:
[https://github.com/apache/commons-math/pull/85|http://example.com/]
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling canIncrement(0) \{ instead of
> canIncrement(nTimes) } to check if the maxCountCallback should be triggered.
> I expect that all uses of the Incrementor actually trigger the
> maxCountCallback when the count has actually exceeded the maximalCount. I
>
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16462262#comment-16462262
]
Alex D Herbert commented on MATH-1458:
--
I've branched the git repo and will create tests that the current
SimpsonItegrator fails. I'll then fix it and submit a pull request for review.
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling canIncrement(0) \{ instead of
> canIncrement(nTimes) } to check if the maxCountCallback should be triggered.
> I expect that all uses of the Incrementor actually trigger the
> maxCountCallback when the count has
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16461450#comment-16461450
]
Gilles commented on MATH-1458:
--
I've created issue MATH-1460. Please have a look.
{quote}I assumed that a core developer would be able to look at this and I had
contributed enough. Are you saying that I have to provide the patch?
{quote}
We are lacking time and expertise to support all the code in this library.
We've started to split it in more manageable parts that can be maintained more
easily by people not necessarily able to fix all the issues that keep coming
for Commons Math and prevent timely releases.
See the new components [Commons RNG|http://commons.apache.org/rng], [Commons
Numbers|http://commons.apache.org/numbers], [Commons
Statistics|http://commons.apache.org/statistics], [Commons
Geometry|http://commons.apache.org/geometry], ... that are at various stages of
completion.
You are most welcome to contribute to e.g. a new module in "Commons Numbers"
that could contain selected parts of the code currently in package
{{org.apache.commons.math4.analysis}}.
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16460916#comment-16460916
]
Alex D Herbert commented on MATH-1458:
--
The comment about the Incrementor is for the new IntegerSequence.Incrementor
not the deprecated org.apache.commons.math3.util.Incrementor.
I think the public void increment(int nTimes) method of
IntegerSequence.Incrementor should be:
{code:java}
public void increment(int nTimes) throws MaxCountExceededException {
if (nTimes <= 0) {
throw new NotStrictlyPositiveException(nTimes);
}
// This is a change from: if (!canIncrement(0)) {
if (!canIncrement(nTimes)) {
maxCountCallback.trigger(maximalCount);
}
count += nTimes * increment;
}
{code}
Since I am new to this please allow some naive questions. I assumed that a core
developer would be able to look at this and I had contributed enough. Are you
saying that I have to provide the patch?
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
>
[jira] [Commented] (MATH-1458) Simpson Integrator computes incorrect value at minimum iterations=1 and wastes an iteration
[
https://issues.apache.org/jira/browse/MATH-1458?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16460872#comment-16460872
]
Gilles commented on MATH-1458:
--
Thanks a lot for the report and thorough analysis.
The next step would be to write a unit test that displays the bug. And then a
patch/PR to fix it.
I don't follow the argument about {{Incrementor}} but please note that it is
_deprecated_ in the development version of the library: updates/fixes should be
performed against the "master" branch in the code repository.
> Simpson Integrator computes incorrect value at minimum iterations=1 and
> wastes an iteration
> ---
>
> Key: MATH-1458
> URL: https://issues.apache.org/jira/browse/MATH-1458
> Project: Commons Math
> Issue Type: Bug
>Affects Versions: 3.6.1
> Environment: openjdk version "1.8.0_162"
>
> OpenJDK Runtime Environment (build 1.8.0_162-8u162-b12-0ubuntu0.16.04.2-b12)
>
> OpenJDK 64-Bit Server VM (build 25.162-b12, mixed mode)
>Reporter: Alex D Herbert
>Priority: Minor
> Labels: documentation, easyfix, newbie, patch
>
> org.apache.commons.math3.analysis.integration.SimpsonIntergrator
> When used with minimalIterationCount == 1 the integrator computes the wrong
> value due to the inlining of computation of stage 1 and stage 0 of the
> TrapezoidIntegrator. Each stage is successive since it relies on the result
> of the previous stage. So stage 0 must be computed first. This inlining
> causes stage 1 to be computed before stage 0:
> {code:java}
> return (4 * qtrap.stage(this, 1) - qtrap.stage(this, 0)) / 3.0;
> {code}
> This can be fixed using:
> {code:java}
> final double s0 = qtrap.stage(this, 0);
> return (4 * qtrap.stage(this, 1) - s0) / 3.0;{code}
> What is not clear is why setting minimum iterations to 1 results in no
> iteration. This is not documented. I would expect setting it to 1 would
> compute the first Simpson sum and then perform 1 refinement. This would make
> it functionality equivalent to the other Integrator classes which compute two
> sums for the first iteration and allow them to be compared if minimum
> iterations = 1. If convergence fails then each additional iteration computes
> an additional sum.
> Note when used with minimalIterationCount > 1 the SimpsonIntegrator wastes a
> stage since it computes the following stages: 0, 0, 1, 2, 3. i.e. stage 0 is
> computed twice. This is because the iteration is incremented after the stage
> is computed:
> {code:java}
> final double t = qtrap.stage(this, getIterations());
> incrementCount();
> {code}
> This should be:
> {code:java}
> incrementCount();
> final double t = qtrap.stage(this, getIterations());
> {code}
> On the first iteration it thus computes the trapezoid sum and compares it to
> zero. This would result in a bad computation if integrating a function whose
> trapezoid sum is zero (e.g. y=x^3 in the range -1 to 1). However since
> iteration only occurs for minimalIterationCount>1 no termination comparison
> is made on the first loop. The first termination comparison can be made at
> iteration=2 where the comparison will be between the Trapezoid sum and the
> first Simpson sum. This is a bug.
> However I do not want to submit a formal patch as there is a lack of
> consistency across all the integrators in their doIntegrate() method with the
> use of incrementCount() and what the iteration number should be at the start
> of the while (true) loop:
> * IterativeLegendreGauss integrator uses getIterations()+1 to mark the
> current iteration inside the loop and calls incrementCount() at the end.
> * TrapezoidIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() at
> the end.
> * The MidpointIntegrator calls incrementCount() at the start of the loop and
> uses getIterations() to mark the current iteration.
> * The RombergIntegrator calls incrementCount() outside the loop, uses
> getIterations() to mark the current iteration and calls incrementCount() in
> the middle of the loop before termination conditions have been checked. This
> allows it to fail when the iterations are equal to the maximum iterations
> even if convergence has been achieved (see Note*).
> * The SimpsonIntegrator uses getIterations() to mark the current iteration
> and calls incrementCount() immediately after.
> Note*: This may not be discovered in a unit test since the incrementCount()
> uses a backing Incrementor where the Incrementor.increment() method calls
> Incrementor.increment(1) which ends up calling
