[
https://issues.apache.org/jira/browse/MATH-1689?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18082557#comment-18082557
]
Ruiqi Dong commented on MATH-1689:
----------------------------------
Thanks [~aherbert] , agreed. The JDK precedent point convinces me. Closing this
from my side. Thanks for considering updating the Javadoc.
> Frequency.equals()/hashCode() ignores the comparator
> ----------------------------------------------------
>
> Key: MATH-1689
> URL: https://issues.apache.org/jira/browse/MATH-1689
> Project: Commons Math
> Issue Type: Bug
> Components: legacy
> Reporter: Ruiqi Dong
> Priority: Minor
>
> *Summary*
> Frequency behavior depends on the comparator used by its internal map, but
> equals() and hashCode() ignore that comparator. Two Frequency objects with
> different ordering semantics can therefore compare equal.
> *Affected code*
> File:
> commons-math-legacy/src/main/java/org/apache/commons/math4/legacy/stat/Frequency.java
> {code:java}
> public long getCumFreq(T v) {
> ...
> Comparator<? super T> c = freqTable.comparator();
> if (c == null) {
> c = new NaturalComparator<>();
> }
> ...
> }
> public List<T> getMode() {
> ...
> }
> @Override
> public int hashCode() {
> final int prime = 31;
> int result = 1;
> result = prime * result + ((freqTable == null) ? 0 :
> freqTable.hashCode());
> return result;
> }
> @Override
> public boolean equals(Object obj) {
> ...
> Frequency<?> other = (Frequency<?>) obj;
> return freqTable.equals(other.freqTable);
> } {code}
>
> *Reproducer*
> Add the following test to
> commons-math-legacy/src/test/java/org/apache/commons/math4/legacy/stat/FrequencyTest.java:
> {code:java}
> @Test
> public void testEqualsShouldConsiderComparator() {
> Frequency<Integer> ascending = new Frequency<>();
> Frequency<Integer> descending = new
> Frequency<Integer>(Comparator.reverseOrder());
> ascending.addValue(1);
> ascending.addValue(2);
> descending.addValue(1);
> descending.addValue(2);
> Assert.assertEquals(1, ascending.getCumFreq(1));
> Assert.assertEquals(2, descending.getCumFreq(1));
> Assert.assertEquals("[1, 2]", ascending.getMode().toString());
> Assert.assertEquals("[2, 1]", descending.getMode().toString());
> Assert.assertNotEquals(ascending, descending);
> } {code}
> Run:
> {code:java}
> mvn -q -pl commons-math-legacy
> -Dtest=org.apache.commons.math4.legacy.stat.FrequencyTest test {code}
> Observed behavior:
> {code:java}
> FrequencyTest.testEqualsShouldConsiderComparator:223
> Values should be different. Actual:
> Value Freq. Pct. Cum Pct.
> 2 1 50% 50%
> 1 1 50% 100% {code}
> Expected behavior:
> Frequencies with different comparators should not compare equal when their
> observable ordering behavior differs.
>
> The comparator affects at least getCumFreq() and getMode(). Since those
> outputs differ, equality and hashing should not ignore the comparator.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
