Skip to content

[caffe2][nomnigraph] Allow empty storage for the 'Edge' class. #7595

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.

Sign up for GitHub

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

Jump to bottom
Merged
bwasti merged 1 commit into from
May 23, 2018
Merged

[caffe2][nomnigraph] Allow empty storage for the 'Edge' class. #7595

bwasti merged 1 commit into from
May 23, 2018

Conversation

@yyetim
Copy link
Contributor

@yyetim yyetim commented May 15, 2018

Summary:
This diff:

  • Changes the Graph, Edge, etc. classes to allow a void type.
  • Refactors Algorithm.h to get rid of unnecessary int placeholder.

Test Plan:

  • There are already both types of uses for Edge, and hence both
    template paths are covered. I.e., all unit tests passed => template
    codes covered.
  • For scc: Now have two tests for Tarjan's impl.

@onnxbot onnxbot added the caffe2 label May 15, 2018
@bwasti bwasti changed the title Allow empty storage for the 'Edge' class. [caffe2][nomnigraph] Allow empty storage for the 'Edge' class. May 15, 2018
@bwasti bwasti requested review from bwasti and yinghai May 15, 2018 21:58
bwasti
bwasti requested changes May 15, 2018
View reviewed changes
caffe2/core/nomnigraph/include/nomnigraph/Graph/Graph.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/core/nomnigraph/include/nomnigraph/Representations/NeuralNet.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

bwasti
bwasti approved these changes May 21, 2018
View reviewed changes
Copy link
Contributor

@bwasti bwasti left a comment

Choose a reason for hiding this comment

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

looks great -- happy to accept this. make sure all the tests pass before landing

caffe2/core/nomnigraph/include/nomnigraph/Graph/Graph.h Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/core/nomnigraph/include/nomnigraph/Graph/TarjansImpl.h Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/core/nomnigraph/include/nomnigraph/Graph/BinaryMatchImpl.h Outdated

This comment was marked as off-topic.

Sign in to view

@yyetim
Allow empty storage for the 'Edge' class.
253242f 
This commit:
- Converts edge storage to an optional type.
- Adds a new test in tarjans_test.
- Refactors related bits in other files.
@yyetim
Copy link
Contributor Author

yyetim commented May 23, 2018

Closing this request: All the tests are currently passing except 1. The build error doesn't seem related.

@yyetim yyetim closed this May 23, 2018
@yyetim yyetim reopened this May 23, 2018
@bwasti bwasti merged commit 42134ee into pytorch:master May 23, 2018
petrex pushed a commit to petrex/pytorch that referenced this pull request May 23, 2018
Merge branch 'caffe2_core_hip' of github.com:petrex/pytorch into caff…
e7a80cd 
…e2_core_hip

* 'caffe2_core_hip' of github.com:petrex/pytorch: (24 commits)
  Allow empty storage for the 'Edge' class. (pytorch#7595)
  Process group base class and Gloo implementation (pytorch#7628)
  _LRSchedulers getstate include optimizer info (pytorch#7757)
  [PyTorch] [gradcheck] change backward() to grad() (pytorch#7710)
  Update test_nn.py (pytorch#7787)
  Define general default scheduler for TBB and fix ppc64le bug (pytorch#7761)
  Add support for accepting Tensor as input in clip_grad_*  functions. (pytorch#7769)
  [Easy] Remove unused code (pytorch#7782)
  Update tbb (pytorch#7734)
  Add @generated annotation (pytorch#7780)
  fix legacy comment after variable tensor merge (pytorch#7771)
  Revert pytorch#7750 and pytorch#7762 to fix Windows CI on master (pytorch#7772)
  Temporarily disable build env check (pytorch#7768)
  Add missing brace (pytorch#7762)
  [C++ API] Add backward() to Tensor and Variable  (pytorch#7750)
  [auto] Update onnx to d43b550 - Fix .gitignore and add missing files (onnx/onnx#1005) onnx/onnx@d43b550
  [auto] Update onnx to ea1aa13 - add tests for reduce ops (onnx/onnx#675) onnx/onnx@ea1aa13
  include cudnn_h (pytorch#7749)
  [C++ API] Using new registration mechanism (pytorch#7663)
  [auto] Update onnx to 5dd68e6 - Add a util function: polish_model (onnx/onnx#1000) onnx/onnx@5dd68e6
  ...
weiyangfb pushed a commit to weiyangfb/pytorch that referenced this pull request Jun 11, 2018
@yyetim @weiyangfb
Allow empty storage for the 'Edge' class. (pytorch#7595)
420c381 
This commit:
- Converts edge storage to an optional type.
- Adds a new test in tarjans_test.
- Refactors related bits in other files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwasti bwasti bwasti approved these changes

@yinghai yinghai Awaiting requested review from yinghai

Assignees

No one assigned

Labels

caffe2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yyetim @bwasti @onnxbot