Skip to content
This repository was archived by the owner on Aug 20, 2025. It is now read-only.

METRON-426: Stellar does not support scientific notation as a literal #257

Closed
cestella wants to merge 3 commits into from
Closed

METRON-426: Stellar does not support scientific notation as a literal #257

cestella wants to merge 3 commits into from

Conversation

@cestella
Copy link
Member

@cestella cestella commented Sep 16, 2016

Stellar does not support scientific notation for expressing literal doubles. It should.

This is exercised in the unit tests for StellarStatisticsTest and StellarTest directly

@cestella cestella mentioned this pull request Sep 16, 2016
Closed
@cestella
Copy link
Member Author

cestella commented Sep 16, 2016

The issue around the StellarScientificTest change is that merging t-digests is not precisely the same as adding the same data to one t-digest. Whether this is a bug in the t-digest code or whether it's an artifact of the theory, I don't know. Because this test is using low amounts of data, it's prone to a bit of jitter it seems and the compression level specified before isn't sufficient to ensure that the epsilon chosen (1e-2) is sufficient for repeated runs. I tightened up the compression level to 150 (which is still quite sensible) to make sure we can support at least 1e-2 accuracy for normally distributed data across 100 runs of 1000 elements.

justinleet
justinleet reviewed Sep 16, 2016
View reviewed changes
...etron-common/src/main/java/org/apache/metron/common/math/stats/OnlineStatisticsProvider.java
* is usually a smallish (usually < 10) multiple of the compression.
*/
public static final int COMPRESSION = 100;
public static final int COMPRESSION = 150;
Copy link
Contributor

@justinleet justinleet Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the comment to reflect the compression default change?

Copy link
Member Author

@cestella cestella Sep 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

nickwallen
nickwallen reviewed Sep 20, 2016
View reviewed changes
metron-platform/metron-common/src/test/java/org/apache/metron/common/stellar/StellarTest.java Outdated
}
{
String query = "1.2e-3 + 2";
Assert.assertEquals(1.2e-3 + 2, ((Number)run(query, ImmutableMap.of("casey", "casey"))).doubleValue(), 1e-3);
Copy link
Contributor

@nickwallen nickwallen Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ImmutableMap.of("casey","casey")(besides promoting your global brand ;) ) do nothing for these tests, right? It'd be more clear, if we removed those. Everything else looks good. I'm a +1 if we can take care of this small issue.

Copy link
Member Author

@cestella cestella Sep 20, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You got it. :)

@nickwallen
Copy link
Contributor

nickwallen commented Sep 20, 2016

+1 Less ego-driven. Ha.

@nickwallen
Copy link
Contributor

nickwallen commented Sep 21, 2016

@justinleet I think everything you mentioned has been addressed. Are you good with this PR?

@asfgit asfgit closed this in e7fed2d Sep 21, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@nickwallen nickwallen nickwallen left review comments

@justinleet justinleet justinleet left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cestella @nickwallen @justinleet