-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Tutorial Documentation touch-up #155
Conversation
This adds a prod build for the tutorials which will be runnable in a docker container.
This changes the online analysis example to a jupyter notebook that can be run in the docker tutorials container. This also adds a prod Dockerfile which can be run through `make tutorials-prod`
Refactor the tutorials in jupyter notebooks and brush up a few spots of the documentation that needed attention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, it is a major step towards more user-friendly documentation and examples!
Please address just a couple of typos and some sentences which may need to be rephrased, otherwise LGTM
doc/sr_cpp_walkthrough.rst
Outdated
====== | ||
|
||
The following example shows how to store, and use a DL model | ||
in the database with the C++ Client. The model is stored a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"stored a file" -> "stored as a file"
======= | ||
|
||
The example in :ref:`SR CPP Models` shows how to store, and use a PyTorch script | ||
in the database with the C++ Client. The script is stored a file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"stored a file" -> "stored as a file"
doc/sr_cpp_walkthrough.rst
Outdated
.. note:: | ||
The C++ API examples are written | ||
to connect to a Redis cluster database. Update the | ||
``Client`` constructor call to connect to a Redis non-cluster database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be something along the lines of "if you are using a non-cluster database, update the Client
constructor"? Or is it said somewhere else that the Database is not a cluster?
doc/sr_fortran_walkthrough.rst
Outdated
.. note:: | ||
The Fortran API examples are written | ||
to connect to a Redis cluster database. Update the | ||
``Client`` constructor call to connect to a non-cluster Redis instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for the C++ examples:
Shouldn't this be something along the lines of "if you are using a non-cluster database, update the
Client
constructor"? Or is it said somewhere else that the Database is not a cluster?
doc/sr_fortran_walkthrough.rst
Outdated
|
||
type(client_type) :: client | ||
|
||
call client%initialize(.false.) ! Change .true. to false if using a clustered database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment should say the opposite.
filled with random numbers and stored in the | ||
database using ``put_tensor``. This subroutine | ||
requires the user to specify a string used as the | ||
'key' (here: ``send_array``) identifying the tensor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe check with SR team if we should refer to this entity as name
or key
.
doc/sr_fortran_walkthrough.rst
Outdated
each element represents the probability that the data | ||
represents a number from 0-9. | ||
|
||
The next declaration declare the strings that will be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"declare" -> "declares".
"""Create SmartRedis Dataset containing multiple NumPy arrays | ||
to be stored at a single key within the database""" | ||
dataset = Dataset(f"data_{time_step}") | ||
dataset.add_tensor("ux", ux) | ||
dataset.add_tensor("uy", uy) | ||
#dataset.add_tensor("feq", feq) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this line added and then commented out?
Description
The tutorials were not very user friendly as they did not provide a method of execution that was easy to quickly setup. This PR solves that by
make tutorials-prod
command to build the tutorials in a linux container where SmartSim is installedThis PR also fixes a few documentation bugs that were found when going over the tutorials.
No internal code changes.
TODO