REF add a base TF pickling class #7
Conversation
|
BTW, I passed on including a lot more of the MLP base class. It didn't seem like there was a lot of extra overhead in the other bits and the abstraction would come at the cost of clarity, which doesn't (yet) seem worth it. |
| os.unlink(tempfile.name) | ||
|
|
||
| def _build_tf_graph(self): | ||
| """build the TF graph, setup model saving and setup a TF session |
There was a problem hiding this comment.
nitpicking: "build" should be capitalized and the one-liner should end in a period. https://www.python.org/dev/peps/pep-0257/
|
|
||
| @abstractmethod | ||
| def _set_up_graph(self): | ||
| """abstract method to assemble the TF graph for estimator |
There was a problem hiding this comment.
nitpicky PEP-257 issue again: this should be something like Assemble the ... estimator..
| This base class defines a few standard attributes to enable fairly | ||
| transparent pickling of TensorFlow models. | ||
|
|
||
| Upon pickling an object, if the self._is_fitted property is True: |
There was a problem hiding this comment.
"When pickling" instead of "Upon pickling"? "Upon" makes it sound like a side effect rather than part of the pickling process.
|
|
||
| Upon unpickling the object: | ||
|
|
||
| 1. All things in the state of the object are set using self.__dict__ |
There was a problem hiding this comment.
"All variables in the state"?
| """Base class for pickling TensorFlow-based scikit-learn estimators. | ||
|
|
||
| This base class defines a few standard attributes to enable fairly | ||
| transparent pickling of TensorFlow models. |
There was a problem hiding this comment.
It might be worth adding a note that TensorFlow has a custom saving mechanism that makes pickling (and thus use in scikit-learn, etc.) nontrivial.
| To use this base class properly, the child class needs to | ||
|
|
||
| 1. Implement the abstract method self._set_up_graph. This method | ||
| should simply build the required TF ops. |
There was a problem hiding this comment.
I think it'd be more consistent with TF's terminology (cf. here) to say "build the required TF graph" or "build the required TF tensors and operations".
| 1. Implement the abstract method self._set_up_graph. This method | ||
| should simply build the required TF ops. | ||
|
|
||
| 2. Exactly once, instantiate a TF graph at self.graph_ and then |
There was a problem hiding this comment.
It might help to clarify when this is supposed to happen: "Exactly once (e.g., in the fit method)...".
minor: I think the code in these docstrings should be wrapped in backticks, per numpydoc.
| call `state = super().__getstate__()` and then append to the | ||
| `state`. | ||
|
|
||
| See the example below. |
There was a problem hiding this comment.
Perhaps this should also refer readers to the MLP classes? Of course, those are more complicated since there's a base class and subclasses.
|
I just had some minor comments about the docs. The functionality looks good. |
|
@mheilman I have fixed up the doc strings. This is ready for a second look! |
(LGTM) |
This PR adds a base pickling class for TensorFlow scikit-learn style estimators. It abstracts out all of the core functionality that could conceivably (right now) be shared by say a MLP and an autoencoder, for example.
Closes #5