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

Add unit test for MNIST example tfjs translation #3802

Merged
merged 8 commits into from
Jul 8, 2020

Conversation

vvmnnnkv
Copy link
Member

Description

Add unit test that passes if MNIST example training Plan successfully translates to TFJS.
(It doesn't test correctness of translation, though, as it would require running JS engine with TFJS)

Affected Dependencies

This unit test should pass after these are merged:

How has this been tested?

It's unit test on its own.

Checklist

@vvmnnnkv vvmnnnkv requested a review from a team as a code owner June 30, 2020 16:23
@Nolski
Copy link
Contributor

Nolski commented Jul 3, 2020

@vvmnnnkv If you update to 3p0 0.0.10 this should resolve the 2nd issue you mentioned

@vvmnnnkv
Copy link
Member Author

vvmnnnkv commented Jul 8, 2020

@Nolski
Looks like we need this fix: OpenMined/Threepio#109 in threepio.
I've used branch directly in requirements.txt to pass unit test in PySyft.
If threepio is published under new version, requirements.txt needs to be updated.

@vvmnnnkv vvmnnnkv requested a review from Nolski July 8, 2020 07:15
@vvmnnnkv vvmnnnkv added Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes Type: Improvement 📈 Minor improvements not introducing a new feature or requiring a major refactor Type: Testing 🧪 Add testing or improving existing testing of a file, feature, or codebase labels Jul 8, 2020
@codecov
Copy link

codecov bot commented Jul 8, 2020

Codecov Report

Merging #3802 into master will increase coverage by 0.00%.
The diff coverage is 96.77%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3802   +/-   ##
=======================================
  Coverage   95.07%   95.07%           
=======================================
  Files         186      186           
  Lines       18857    18868   +11     
=======================================
+ Hits        17928    17939   +11     
  Misses        929      929           
Impacted Files Coverage Δ
test/execution/test_plan.py 100.00% <ø> (ø)
...frameworks/torch/tensors/interpreters/gradients.py 94.19% <33.33%> (-0.04%) ⬇️
syft/execution/action.py 93.91% <100.00%> (+0.05%) ⬆️
syft/execution/plan.py 94.85% <100.00%> (+0.01%) ⬆️
test/execution/test_translation.py 100.00% <100.00%> (ø)

@cereallarceny cereallarceny self-requested a review July 8, 2020 13:46
@cereallarceny cereallarceny merged commit e330a38 into master Jul 8, 2020
@delete-merged-branch delete-merged-branch bot deleted the vvm/mnist-tfjs-plan branch July 8, 2020 13:46
Nilanshrajput pushed a commit to Nilanshrajput/PySyft that referenced this pull request Jul 15, 2020
* Add unit test for MNIST example tfjs translation

* Fix tfjs translation for mnist training example

* Fix missing import

* Update threepio dependency

* Fix unit test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Review Needed 🙋 This needs someone to approve, deny, comment, or request changes Type: Improvement 📈 Minor improvements not introducing a new feature or requiring a major refactor Type: Testing 🧪 Add testing or improving existing testing of a file, feature, or codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants