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

Fix issues in handle_func_command in syft tensors #3807

Merged
merged 2 commits into from
Jul 7, 2020

Conversation

LaRiffle
Copy link
Contributor

@LaRiffle LaRiffle commented Jul 1, 2020

Description

This cleans code in handle_func_command and fix bugs due to variable overwriting for syft tensors, and to overloading functions in native.py

@LaRiffle LaRiffle added the Type: Bug 🐛 Some functionality not working in the codebase as intended label Jul 1, 2020
@gmuraru
Copy link
Member

gmuraru commented Jul 1, 2020

Q: Could you add a test for this?

@codecov
Copy link

codecov bot commented Jul 7, 2020

Codecov Report

Merging #3807 into master will increase coverage by 0.00%.
The diff coverage is 97.05%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #3807    +/-   ##
========================================
  Coverage   94.69%   94.70%            
========================================
  Files         187      187            
  Lines       18626    18961   +335     
========================================
+ Hits        17638    17957   +319     
- Misses        988     1004    +16     
Impacted Files Coverage Δ
...ft/frameworks/torch/tensors/interpreters/native.py 91.25% <92.30%> (-0.03%) ⬇️
...orks/torch/tensors/interpreters/additive_shared.py 93.43% <100.00%> (+0.01%) ⬆️
.../frameworks/torch/tensors/interpreters/autograd.py 88.23% <100.00%> (+0.12%) ⬆️
...frameworks/torch/tensors/interpreters/precision.py 97.26% <100.00%> (+0.01%) ⬆️
syft/execution/translation/threepio.py 91.04% <0.00%> (-8.96%) ⬇️
syft/execution/computation.py 96.15% <0.00%> (-3.85%) ⬇️
syft/frameworks/torch/he/fv/util/operations.py 90.97% <0.00%> (-2.58%) ⬇️
syft/generic/frameworks/attributes.py 87.75% <0.00%> (-1.61%) ⬇️
syft/exceptions.py 77.63% <0.00%> (-0.55%) ⬇️
syft/frameworks/torch/he/fv/evaluator.py 93.70% <0.00%> (-0.35%) ⬇️
... and 13 more

@LaRiffle
Copy link
Contributor Author

LaRiffle commented Jul 7, 2020

Q: Could you add a test for this?

I've updated the code once more maybe it's clearer now. It's hard to add tests for this, but it's needed for batchnorm that I wanted to put in a separate PR and which is tested. Sounds good?
(and the main function at stakes here, torch.roll, is tested)

@@ -356,7 +372,7 @@ def handle_func_command(cls, command):
# Check that the function has not been overwritten
Copy link
Member

Choose a reason for hiding this comment

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

PICKY Stuff: Here the comment should be removed, right? Since we look for native_{cmd}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

haha no it's good to be picky. here the module native_torch defined in native.py overwrites native torch functions... Yeah it's a bit strange, I really had to modify a torch function (aka torch.roll) to behave slightly differently

Copy link
Member

@gmuraru gmuraru left a comment

Choose a reason for hiding this comment

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

LGTM! Only one small question

@LaRiffle LaRiffle merged commit 5c8e7b7 into master Jul 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the ryffel/fix-handle-cmd branch July 7, 2020 21:57
Nilanshrajput pushed a commit to Nilanshrajput/PySyft that referenced this pull request Jul 15, 2020
* Fix issues in handle_func_command in syft tensors

* Fix handle_func_cmd in native
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug 🐛 Some functionality not working in the codebase as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants