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.

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

613-Support missing aggregation functions in aggregation kernels #638

Merged
merged 20 commits into from
Nov 17, 2023

Conversation

inikokali
Copy link
Contributor

Issue
var is not supported for any of AggAll, AggRow, AggCol and stddev is not supported for full aggregation (AggAll) and row-wise aggregation (AggRow).

Implementation
This pull request proposes implementation for the above aggregation functions following the implementation of MEAN aggregation function in each instance.

Changes

  • Changes in files AggAll.h, AggRow.h, AggCol.h related to the implementation of stddev and var.
  • Updated file AggOpCode.h to add var opCode.
  • Updated file kernels.json to add stddev and var opCodes where needed.
  • Include testing scenarios to ensure the validity of my implementation. There are tests for both stddev and var functions for all three kernels (AggAll, AggRow, AggCol). The tests can be found in the related test cases ( AggAllTest.cpp, AggRowTest.cpp, AggCoTest.cpp ).
  • Updated test cases matrix_agg.daphne and matrix_agg.py.

@pdamme pdamme self-requested a review November 17, 2023 20:23
- Unit test cases for aggAll/aggCol/aggRow-kernels use approximate comparisons now, to avoid failing test cases through minor deviations in floating-point numbers.
- Some minor changes.
Copy link
Collaborator

@pdamme pdamme left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, @inikokali. With that, DAPHNE finally supports the aggregation functions SUM, MIN, MAX, MEAN, VAR, and STDDEV consistently for full, row-wise, and column-wise aggregation. Great!

Your code looks good to me and the tests pass. I only did some minor formatting and, more importantly, adapted the unit test cases to check for approximate equality of floating-point numbers. So far, the tiny example data in these test cases was chosen such that all results are "nice" numbers, without a lot of digits after the point. That's, of course, more difficult to construct for VAR and STDDEV. To avoid future test failures from insignificant floating-point deviations, we do approximate comparisons now.

@pdamme pdamme merged commit ae1d021 into daphne-eu:main Nov 17, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants