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

Implementing VectorSumCombiner #276

Merged
merged 12 commits into from
May 20, 2022

Conversation

rialg
Copy link
Contributor

@rialg rialg commented May 13, 2022

Description

Following the first item described in #264, this PR has the implementation of the VectorSumCombiner class, which should be used to compute vector/array sums.

Affected Dependencies

Needed to extend AggregateParams with the fields norm_kind and max_norm.

How has this been tested?

  • Tests added to combiners_test.py
==================================================================================================================== test session starts =====================================================================================================================
platform linux -- Python 3.9.10, pytest-7.1.2, pluggy-1.0.0
plugins: timeout-2.1.0
collected 45 items                                                                                                                                                                                                                                           

tests/combiners_test.py .............................................                                                                                                                                                                                  [100%]

====================================================================================================================== warnings summary ======================================================================================================================
../.local/lib/python3.9/site-packages/hdfs/config.py:15
DeprecationWarning: the imp module is deprecated in favour of importlib; see the module's documentation for alternative uses
    from imp import load_source

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
=============================================================================================================== 45 passed, 1 warning in 2.20s ================================================================================================================

Checklist

pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good, I left comments

examples/movie_view_ratings/run_all_frameworks.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
Gastón Rial added 2 commits May 17, 2022 12:53
Add error handling to AggregateParams
Update tests
examples/movie_view_ratings/run_all_frameworks.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/combiners.py Show resolved Hide resolved
tests/combiners_test.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thank you! It looks great. Just a few minor comments

pipeline_dp/combiners.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
pipeline_dp/aggregate_params.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@dvadym dvadym left a comment

Choose a reason for hiding this comment

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

Thank you!

@dvadym dvadym merged commit bb40462 into OpenMined:main May 20, 2022
@rialg rialg deleted the vector_summation_dp_aggregation branch May 21, 2022 13:06
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