Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Statistics on a Vector #3442

Merged
merged 21 commits into from
May 11, 2022
Merged

Statistics on a Vector #3442

merged 21 commits into from
May 11, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented May 9, 2022 •
edited
Loading

Pull Request Description

  • Implements various statistics on Vector

Important Notes

Some minor codebase improvements:

  • Some tweaks to Any/Nothing to improve performance
  • Fixed bug in ObjectComparator
  • Added if_nothing
  • Removed Group_By_Key

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

@jdunkerley jdunkerley force-pushed the wip/jd/statistics-182059762 branch 3 times, most recently from 55db3cb to 669f233 Compare May 10, 2022 16:23
@jdunkerley jdunkerley marked this pull request as ready for review May 10, 2022 16:23
@jdunkerley jdunkerley requested review from 4e6 and radeusgd as code owners May 10, 2022 16:23
@jdunkerley jdunkerley force-pushed the wip/jd/statistics-182059762 branch from 3a63e9e to 164d80e Compare May 11, 2022 08:57
hubertp
hubertp approved these changes May 11, 2022
View reviewed changes
Copy link
Collaborator

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nits

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Any.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Nothing.enso Outdated Show resolved Hide resolved
radeusgd
radeusgd reviewed May 11, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks good to me, I really like the approach to computing the moment-based statistics.

Some style suggestions that I think should be addressed below:

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Vector.enso Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Nothing.enso Outdated Show resolved Hide resolved
std-bits/base/src/main/java/org/enso/base/statistics/CountMinMax.java Outdated Show resolved Hide resolved
test/Tests/src/Data/Statistics_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Statistics_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Statistics_Spec.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Statistics.enso Outdated Show resolved Hide resolved
std-bits/base/src/main/java/org/enso/base/statistics/Statistic.java Outdated Show resolved Hide resolved
@jdunkerley jdunkerley requested a review from radeusgd May 11, 2022 11:56
@jdunkerley jdunkerley force-pushed the wip/jd/statistics-182059762 branch from 9e49faa to 5f45e3d Compare May 11, 2022 11:57
radeusgd
radeusgd approved these changes May 11, 2022
View reviewed changes
test/Tests/src/Data/Statistics_Spec.enso Outdated Show resolved Hide resolved
test/Tests/src/Data/Statistics_Spec.enso Outdated Show resolved Hide resolved
std-bits/base/src/main/java/org/enso/base/statistics/Moments.java Outdated Show resolved Hide resolved
@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label May 11, 2022
@mergify mergify bot merged commit 4f3a768 into develop May 11, 2022
@mergify mergify bot deleted the wip/jd/statistics-182059762 branch May 11, 2022 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@hubertp hubertp hubertp approved these changes

@4e6 4e6 Awaiting requested review from 4e6

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jdunkerley @hubertp @radeusgd