Skip to content

Algorithm rand bindings#123

Merged
chinmayshah99 merged 15 commits intoOpenMined:devfrom
leclair-7:algorithm_rand_bindings
May 7, 2020
Merged

Algorithm rand bindings#123
chinmayshah99 merged 15 commits intoOpenMined:devfrom
leclair-7:algorithm_rand_bindings

Conversation

@leclair-7
Copy link
Copy Markdown
Contributor

Description

Binds algorithms/rand to pydp.

Please mark options that are relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • [ x] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I ran build PyDP.sh, ran python on my terminal:
import pydp as dp
dp.Geometric()
dp.UniformDouble()

Numbers matched what rand.h indicates they should be.

Comment thread tests/algorithms/test_rand.py
@leclair-7
Copy link
Copy Markdown
Contributor Author

Ok, I've added instance checks to everything currently binded except utils. For some reason to import it, it has to be "import pydp.pydp.util" which I don't think is intended.

alejandrosame
alejandrosame previously approved these changes May 5, 2020
Copy link
Copy Markdown
Member

@alejandrosame alejandrosame left a comment

Choose a reason for hiding this comment

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

I approve the PR, but I want to raise awareness that it is bad practice to make PRs (and generally group commits) that span files unrelated to the feature that the PR targets.

In this case, the addition of isinstance tests for the other classes should be made in a separate PR to make the scope clearer. It's not critical at all right now, but at some point I expect that we will need to reinforce this kind of workflows.

Comment thread tests/algorithms/test_algorithms.py Outdated
Copy link
Copy Markdown
Member

@chinmayshah99 chinmayshah99 left a comment

Choose a reason for hiding this comment

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

LGTM!

@chinmayshah99 chinmayshah99 merged commit 20d7b06 into OpenMined:dev May 7, 2020
dvadym pushed a commit to dvadym/PyDP that referenced this pull request Jul 3, 2022
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.

4 participants