You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "Sebb (JIRA)" <ji...@apache.org> on 2011/02/01 19:38:29 UTC
[jira] Created: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
The static field ChiSquareTestImpl.distribution serves no purpose
-----------------------------------------------------------------
Key: MATH-506
URL: https://issues.apache.org/jira/browse/MATH-506
Project: Commons Math
Issue Type: Bug
Affects Versions: 2.1, 1.2
Reporter: Sebb
Priority: Minor
The static field ChiSquareTestImpl.distribution serves no purpose.
There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
For Math 1.x, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Resolved] (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Phil Steitz resolved MATH-506.
------------------------------
Resolution: Fixed
I removed the field (hence eliminating pluggability) in r1159916. As of 3.0, the distribution classes are immutable, so to support pluggability a factory or class name rather than a distribution instance would have to be provided. There is only one implementation provided by [math], so I do not see this as worth the effort and complexity to retain.
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.0, 2.1, 2.2
> Reporter: Sebb
> Assignee: Phil Steitz
> Priority: Minor
> Fix For: 3.0
>
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12991006#comment-12991006 ]
Sebb commented on MATH-506:
---------------------------
OK, I see now, thanks!
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Issue Comment Edited: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12990981#comment-12990981 ]
Sebb edited comment on MATH-506 at 2/5/11 1:24 PM:
---------------------------------------------------
Just tried removing the field and setter in 3.0, and found that the constructors rely on the setter (which is a separate bug, as the setter is not final - but easily fixable if required).
The fix for MATH-349 merely removed deprecated code.
It replaced "distribution.setDegreesOfFreedom(dof)" with "distribution = new ChiSquaredDistributionImpl(dof)" which is how the field became useless.
There are two constructors which still create values for the distribution field.
I don't know enough about the Math to know whether there would be any use cases for having additional methods that used a distribution provided by the class instance, rather than calculated by the individual methods (as at present).
If there is no need for external provision of the distribution degree of freedom, then the constructor with parameter can be dropped.
Otherwise, we need to add some methods that can use the provided distribution (which should be a final instance field).
In any case, I think the setter needs to be dropped from 3.x
was (Author: sebb@apache.org):
Just tried removing the field and setter in 3.0, and found that the constructors rely on the setter (which is a separate bug, as the setter is not final - but easily fixable if required).
The fix for MATH-349 merely removed deprecated code.
It replaced "distribution.setDegreesOfFreedom(x)" with "distribution = new ChiSquaredDistributionImpl(x)" which is how the field became useless.
There are two constructors which still create values for the distribution field.
I don't know enough about the Math to know whether there would be any use cases for having additional methods that used a distribution provided by the class instance, rather than calculated by the individual methods (as at present).
If there is no need for external provision of the distribution degree of freedom, then the constructor with parameter can be dropped.
Otherwise, we need to add some methods that can use the provided distribution (which should be a final instance field).
In any case, I think the setter needs to be dropped from 3.x
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12990893#comment-12990893 ]
Phil Steitz commented on MATH-506:
----------------------------------
Agreed. Since the fix for MATH-349 this instance field is unnecessary.
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 2.1, 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] [Updated] (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Phil Steitz updated MATH-506:
-----------------------------
Affects Version/s: (was: 3.0)
1.2
2.0
2.1
2.2
Fix Version/s: 3.0
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 1.2, 2.0, 2.1, 2.2
> Reporter: Sebb
> Assignee: Phil Steitz
> Priority: Minor
> Fix For: 3.0
>
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sebb updated MATH-506:
----------------------
Description:
The static field ChiSquareTestImpl.distribution serves no purpose.
There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
For Math 2.1, the field can be removed and the setter deprecated.
was:
The static field ChiSquareTestImpl.distribution serves no purpose.
There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
For Math 1.x, the field can be removed and the setter deprecated.
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 2.1, 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12990973#comment-12990973 ]
Sebb commented on MATH-506:
---------------------------
Sorry - I thought I had checked the 2.x implementation as well, but obviously not, as it does use the field.
However, we should still deprecate the setter in 2.2, as it is removed in 3.0 - OK?
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12990995#comment-12990995 ]
Phil Steitz commented on MATH-506:
----------------------------------
The instance field was there originally so that different ChiSquareDistribution implementations could be provided at construction time or via a setter (making the underlying ChiSquareDistribution pluggable). MATH-349 pointed to a different problem related to mutability of implementation instances. The simplest solution to both problems is to eliminate the pluggability, which the change in MATH-349 does for this class. The degrees of freedom are always computed from the data, so there is no need for the constructor that takes a distribution instance as argument. Both the constructor and setter can be deprecated in 2.2 and removed in 3.0 unless we want to keep pluggability, which would require
1) making the distribution field final (so removing the setter)
2) copying, rather than referencing the actual parameter provided to the constructor
I am on the fence on this. Maybe others can chime in (next week :)
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Phil Steitz (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Phil Steitz updated MATH-506:
-----------------------------
Affects Version/s: (was: 2.1)
See the discussion in MATH-349 where it was decided to remove the distribution pluggability in 3.0. In 2.x, the distribution is pluggable and the instance field is useful. The 3.0 code in trunk removes the pluggability and makes the field useless.
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Commented: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12990981#comment-12990981 ]
Sebb commented on MATH-506:
---------------------------
Just tried removing the field and setter in 3.0, and found that the constructors rely on the setter (which is a separate bug, as the setter is not final - but easily fixable if required).
The fix for MATH-349 merely removed deprecated code.
It replaced "distribution.setDegreesOfFreedom(x)" with "distribution = new ChiSquaredDistributionImpl(x)" which is how the field became useless.
There are two constructors which still create values for the distribution field.
I don't know enough about the Math to know whether there would be any use cases for having additional methods that used a distribution provided by the class instance, rather than calculated by the individual methods (as at present).
If there is no need for external provision of the distribution degree of freedom, then the constructor with parameter can be dropped.
Otherwise, we need to add some methods that can use the provided distribution (which should be a final instance field).
In any case, I think the setter needs to be dropped from 3.x
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 2.1, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira
[jira] Updated: (MATH-506) The static field
ChiSquareTestImpl.distribution serves no purpose
Posted by "Sebb (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/MATH-506?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Sebb updated MATH-506:
----------------------
Affects Version/s: (was: 1.2)
3.0
> The static field ChiSquareTestImpl.distribution serves no purpose
> -----------------------------------------------------------------
>
> Key: MATH-506
> URL: https://issues.apache.org/jira/browse/MATH-506
> Project: Commons Math
> Issue Type: Bug
> Affects Versions: 2.1, 3.0
> Reporter: Sebb
> Priority: Minor
>
> The static field ChiSquareTestImpl.distribution serves no purpose.
> There is a setter for it, but in every case where the field is used, it is first overwritten with a new value.
> The field and the setter should be removed, and the methods that create a new instance should create a local variable instead.
> For Math 1.x, the field can be removed and the setter deprecated.
--
This message is automatically generated by JIRA.
-
For more information on JIRA, see: http://www.atlassian.com/software/jira