-
Notifications
You must be signed in to change notification settings - Fork 3
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
first readme #2
first readme #2
Conversation
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.
Thank you for writing this, looks very good. I wrote some comments below but for the most part I can also work on these as I am updating the benchmark script in the other PR. Just let me know your opinion particularly on how to document the main.py
arguments.
Something we could potentially do is move the main
method which contains the benchmark code in a separate file and leave only the ArgumentParser
(which should be well documented using help=
for each argument) and the execution on main.py
. This would also allow us to write tests for the main
method.
README.md
Outdated
- GPUs are the default devices for qibojit and qibotf. If you need CPU performance numbers do `export CUDA_VISIBLES_DEVICE=""` before executing the benchmark script. | ||
- CPU simulations by default use physical cores as number of threads with qibojit and qibotf. To control this behaviour without touching the code do `export OMP_NUM_THREADS=<threads>` before executing the bencharmk script. | ||
- The benchmark script provides several options, including the possibility to modify the default numba threading pooling technology. |
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.
I am not sure if we would like to elaborate a bit more on these options, for example what exactly the numba threading flag controls, the GPU memory limiter for tensorflow, etc.
We should definitely document well all arguments that are available in main.py
. This can be done either here on the README directly or using the help
argument in the parser.add_argument
definitions.
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.
- simulation_time_std: standard deviation of simulation_time in seconds. | ||
- transfer_time: average transfer time of results from GPU to CPU for `nreps` repetitions in seconds. | ||
- transfer_time_std: standard deviation of transfer_time in seconds. |
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.
Not very important but the time to import qibo is also logged and in case the user performs measurements using the --nshots
flag the measurement time is also logged.
We should also mention somewhere that if the --filename
flag is used these logs and times are also saved on disk in a json format.
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, I think we should include that on the argparse help.
README.md
Outdated
|
||
- `qft`: [quantum fourier transform](https://en.wikipedia.org/wiki/Quantum_Fourier_transform) | ||
- `variational_circuit`: [variational quantum circuit layer as defined](https://qibo.readthedocs.io/en/latest/qibo.html#variational-layer) |
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.
I think there is something missing in the variational circuit sentence, perhaps the correct would be "as defined in (link to qibo docs)".
Also this is not exactly correct because as it is now in the benchmark script the variational circuit offers two options:
- one using the variational layer
- one using plain RY and CZ gates
and the user can switch between the two using a flag. Although it may be interesting to check again if the VariationalLayer
gate offers any advantage (it should), this detail may be out of scope for the qibojit paper where the focus is to compare different backends.
@stavros11 thanks, let me propose to merge this in your branch, and start inviting the nvidia team. |
Implements a basic readme.