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

make computationgraph constructor private to force use of factory method #47

Merged
merged 2 commits into from Feb 23, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 8 additions & 18 deletions swig/README.md
Expand Up @@ -149,7 +149,7 @@ it also on CPU. If you just use the Scala `myInitialize()` helper you should be

## Differences between Scala and C++

### Delete Your ComputationGraphs
### `ComputationGraph.getNew`

DyNet does not like it if you try to instantiate more than one ComputationGraph at a time.

Expand All @@ -162,27 +162,26 @@ for (int i = 0; i < NUM_TIMES; i++) {
}
```

This works because here `cg` gets destructed each time it goes out of scope. The same is not true in Scala
This works because here `cg` gets destructed each time it goes out of scope. The same is not true in Scala. If you were to try something like

```scala
// BAD! DO NOT DO THIS!
// THIS CODE WILL NOT RUN

Choose a reason for hiding this comment

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

Maybe just delete the paragraph on new ComputationGraph now since it's irrelevant?

for (i <- 0 until NUM_TIMES) {
val cg = new ComputationGraph
// do some computations
}
```

The underlying C++ ComputationGraph gets destructed at some point
the underlying C++ ComputationGraph would get destructed at some point
(presumably whenever the Java GC runs),
but not immediately. As a result, your program will crash with the dreaded
but not immediately. As a result, your program would crash with the dreaded

```
[error] Memory allocator assumes only a single ComputationGraph at a time.
```

To avoid this, we added a static `getNew` method that remembers the
last requested ComputationGraph and deletes it every time you request
another.
To prevent this, the Scala bindings are designed so that
you can only get new `ComputationGraph`s using the static `getNew` method:

```scala
for (i <- 0 until NUM_TIMES) {
Expand All @@ -191,16 +190,7 @@ for (i <- 0 until NUM_TIMES) {
}
```

Alternatively, you can manually call `.delete()` on each
`ComputationGraph` instance when you are done with it:

```scala
for (i <- 0 until NUM_TIMES) {
val cg = new ComputationGraph
// do some computations
cg.delete()
}
```
which keeps track of the previously allocated `ComputationGraph` and deletes it for you.

### `std::vector`s

Expand Down
3 changes: 3 additions & 0 deletions swig/dynet_swig.i
Expand Up @@ -631,11 +631,14 @@ Expression trace_of_product(const Expression& x, const Expression& y);
if (singletonInstance != null) {
singletonInstance.delete();
}

singletonInstance = new ComputationGraph();
return singletonInstance;
}
%}

%javamethodmodifiers ComputationGraph::ComputationGraph() "private";

struct ComputationGraph {
ComputationGraph();
~ComputationGraph();
Expand Down
2 changes: 1 addition & 1 deletion swig/src/main/java/edu/cmu/dynet/examples/XorExample.java
Expand Up @@ -24,7 +24,7 @@ public static void main(String[] args) {
System.out.println("Dynet initialized!");
Model m = new Model();
SimpleSGDTrainer sgd = new SimpleSGDTrainer(m);
ComputationGraph cg = new ComputationGraph();
ComputationGraph cg = ComputationGraph.getNew();

// Declare parameters
Parameter p_W = m.add_parameters(makeDim(new int[]{HIDDEN_SIZE, 2}));
Expand Down