Added documentation #26
Conversation
|
@anshuman23 we should not add the |
lib/tensorflex.ex
Outdated
| - Since Tensorflex provides Inference capability for pre-trained graph models, it is assumed you have adequate knowledge of the pre-trained models you are using (such as the input data type/dimensions, input and output operation names, etc.). Some basic understanding of the [Tensorflow Python API](https://www.tensorflow.org/api_docs/python/) can come in very handy. | ||
|
|
||
| - Tensorflex consists of multiple NIFs, so exercise caution while using it-- providing incorrect operation names for running sessions, incorrect dimensions of tensors than the actual pre-trained graph requires, providing different tensor datatypes than the ones required by the graph can all lead to failure. While these are not easy errors to make, do ensure that you test your solution well before deployment. | ||
| """ |
There was a problem hiding this comment.
Can we wrap this and all docs below around 80-100 columns?
Otherwise reading them becomes a bit hard. :)
There was a problem hiding this comment.
Yes, I apologize, I should have done that before committing. I've wrapped all the doc text to 80 columns now :)
| @@ -1,5 +1,6 @@ | |||
| defmodule Tensorflex.Tensor do | |||
|
|
|||
| @moduledoc false | |||
There was a problem hiding this comment.
Maybe this should be public given that we return it in many examples?
There was a problem hiding this comment.
I think that might be counter-intuitive. Since the structs mainly handle the NIF references, I don't think it would be appropriate for users to know about the internal working of the Tensorflex structs. Adding docs for the internal structs might make users think that they can also modify/work with them. Plus there is no code to really document, just the reference wrapping struct.
This is my take on it. What do you think?
PS: I'm merging this PR for now, but in case you think that we should have docs for the structs, I will put in a new PR for them
There was a problem hiding this comment.
So the best is to implement Inspect protocol for this to return something opaque. Something like this after the module definition:
defimpl Inspect, for: Tensorflex.Tensor do
def inspect(data, _opts) do
"#Tensorflex.Tensor<#{data.datatype}>"
end
end
There was a problem hiding this comment.
This would be the best thing to do. I'll add this code to all these side modules in the next PR. Thank you!
lib/tensorflex.ex
Outdated
| Generally to check that the loaded graph model is correct and contains computational operations, the `get_graph_ops/1` function is useful: | ||
| ```elixir | ||
| iex(2)> Tensorflex.get_graph_ops graph | ||
| ["DecodeJpeg/contents", "DecodeJpeg", "Cast", "ExpandDims/dim", "ExpandDims", |
There was a problem hiding this comment.
Shouldn we indent this result in the same way IEx idents it when printing to the shell?
There was a problem hiding this comment.
It is indented in exactly the same way the IEx prints the output to shell. I have copied these outputs from there, after running their commands
|
Thanks @anshuman23! I have added some comments, some are aesthetic ones, but generally this looks great! |
lib/tensorflex.ex
Outdated
| different tensor datatypes than the ones required by the graph can all lead to | ||
| failure. While these are not easy errors to make, do ensure that you test your | ||
| solution well before deployment. | ||
| """ |
There was a problem hiding this comment.
The standard in Elixir is to keep all of the contents inside the heredoc indented. SOmething like this:
defmodule Foo do
@moduledoc """
Foo bar is a very long line
that is meant to work as example:
* this is a bullet that spans more
than one line
"""
def bar doThere was a problem hiding this comment.
Alright I see. This would look better and be more readable.
Would it be possible to share what tool/editor you use for doing this? I use Vim and/or Emacs and I am not sure how to do this using either of them. I used Vim for currently wrapping the text to the 80 column limit.
There was a problem hiding this comment.
@josevalim I have made the IEx indentation changes. This is the only change remaining. How can I fix this?
There was a problem hiding this comment.
I use sublime text 3 but it doesn't do auto formatting. It does help me keep the proper indention though.
There was a problem hiding this comment.
I fixed this so the indentation reads well now
lib/tensorflex.ex
Outdated
| "conv_2/control_dependency", "conv_2", "pool/CheckNumerics", | ||
| "pool/control_dependency", "pool", "conv_3/conv2d_params", "conv_3/Conv2D", | ||
| "conv_3/batchnorm/beta", "conv_3/batchnorm/gamma", | ||
| "conv_3/batchnorm/moving_mean", "conv_3/batchnorm/moving_variance", ...] |
There was a problem hiding this comment.
I think when pasting the code here you lost the indentation. Here are the examples above from my IEx shell:
There was a problem hiding this comment.
This screenshot is from the generated html doc from the current code itself. To me, the output looks exactly similar to the output image you uploaded. I hope I am not misunderstanding what you're saying.
There was a problem hiding this comment.
@anshuman23 you can see that all lines in my example are indented by one, according to the list start.
But this is minor, the biggest issue is the struct example I posted above. Where the struct elements should be indented by at least two spaces.
There was a problem hiding this comment.
I see. I'm sorry I didn't notice the indentation-- I thought there was something wrong with the way the text was presented. I think you are right, copying from the shell probably led to a loss in the formatting.
I will fix this right away. Thanks for noticing this problem! :)
|
@josevalim this is somewhat unrelated, but I had also modified the README directly to read well directly using Github markdown editor. Please let me know if you find something amiss from there as well. |
|
Don't forget to add |
@josevalim I have added documentation using ExDoc. Please let me know what all changes you think are required before I merge this.
Next I will clean up the README.