-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implementation of CUDA accelerated passive crossbar programming simulation for the 2021 Data Driven model #125
Conversation
The following variable: random_crossbar_init initializes the crossbar to random device conductances in between 1/Ron and 1/Roff. This will be useful to test the robustness of a passive crossbar to reprogram itself to different conductances after previous inference. It will not affect anything if transistor is set to True.
# Conflicts: # memtorch/bh/crossbar/Crossbar.py # memtorch/mn/Module.py
not tested not working just backup for now
Implementation of data_driven simulate no neighbours made.
Shaky accuracy with simulate neighbours routine (probably working as intended, but the effects of V/2 applied is non neglectable) Documentation added for the cuda kernels
Changed the code of crossbar.py based on what is currently implemented in CUDA
The following variable: random_crossbar_init initializes the crossbar to random device conductances in between 1/Ron and 1/Roff. This will be useful to test the robustness of a passive crossbar to reprogram itself to different conductances after previous inference. It will not affect anything if transistor is set to True.
not tested not working just backup for now
Implementation of data_driven simulate no neighbours made.
Shaky accuracy with simulate neighbours routine (probably working as intended, but the effects of V/2 applied is non neglectable) Documentation added for the cuda kernels
Changed the code of crossbar.py based on what is currently implemented in CUDA
The code would have called memtorch_cuda_bindings even if the version was CPU.
This pull request introduces 2 alerts and fixes 2 when merging de8dc2d into aec1e25 - view on LGTM.com new alerts:
fixed alerts:
|
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 your contribution! This functionality looks great.
I have made a number of minor comments/suggestions for your consideration. Your pull request can be updated by pushing new commit(s) to 3it-nano:master directly. The --no-verify
flag can be used alongside git commit
to bypass pre-commit workflows. It would be greatly appreciated if you could update setup.py
so that the automated CI workflow can run in full.
I'm definitely happy to add input arguments to specify the maximum voltage incrementation/decrementation of the amplitude of the voltage used to program devices, and for the failure iteration threshold once this is merged to master
. This, alongside (#123), can makeup the 1.1.6 release. I can sort the newly required documentation, changelog, and other miscellaneous changes on my end.
Regarding plotting utility files: In memtorch.bh.memristor.Memristor
, I originally defined two abstract methods for plotting IV characteristics and bipolar switching behaviour: plot_hysteresis_loop
, and plot_bipolar_switching_behaviour
.
I would suggest developing common plotting functions/utilities under a new submodule entitled memtorch.plot
(to more easily standardize things such as the font style used and other properties), which can be called from other methods. For example, memtorch.bh.crossbar.Crossbar.plot_conductance_matrices
could be be defined, which calls memtorch.plot.plot_heatmat
. I've tentatively created a new issue (#126) detailing this. Feel free to reach out if you need any help with this. I'm happy to develop the associated documentation if you would like!
if (instruction_array[k] != 0) | ||
{ | ||
//Check to ensure that the resistance remains within possible range | ||
if (resistance_ > r_off_global) |
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.
This logic can be moved outside of the if else if
clause, i.e., it is used if (i == current_i && j == current_j) || (i == current_i || j == current_j)
.
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 adapted the code with respect to this suggestion
Co-authored-by: Corey Lammie <[email protected]>
Set CUDA = False for CI Co-authored-by: Corey Lammie <[email protected]>
remove memristor_model_params random comment oversight Co-authored-by: Corey Lammie <[email protected]>
Co-authored-by: Corey Lammie <[email protected]>
Co-authored-by: Corey Lammie <[email protected]>
Co-authored-by: Corey Lammie <[email protected]>
Co-authored-by: Corey Lammie <[email protected]>
Co-authored-by: Corey Lammie <[email protected]>
This pull request introduces 1 alert when merging 054e5dc into aec1e25 - view on LGTM.com new alerts:
|
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
- Coverage 84.53% 84.20% -0.33%
==========================================
Files 54 54
Lines 2276 2292 +16
==========================================
+ Hits 1924 1930 +6
- Misses 352 362 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Thank you for your swift response and for these comments! I will solve them now and commit my plotting functions later on (following your suggestions) . Its great that you do not mind taking care of the documentation, do not hesitate if you have any questions or if you want me to take it in my charge. I resolved some comments, please feel free to reopen them if you feel that the solution provided is unsatisfactory I am sorry for the many commits to the branch that trigger the bot verification everytime! |
- Removed ideal_crossbar matrix - Made list of implemented models global - Added exception thrown when a model is not implemented - Refactored a part of the CUDA kernel -Removed function from CUD header (cuh file) -Removed redundant build_g_tensor function
No problem at all! All of your changes look good to me- thank you. I'll merge this to The new plotting functionality can be included in a future release. Don't hesitate to reach out if you have any questions! |
Partial solution to issue #53
The added cuda files are built alongside the other cuda extensions if CUDA is set to true in setup.py. The cpp bindings have not yet been implemented and only the data-driven model is implemented so far for the cuda bindings. Crossbar.py has been modified to call the bindings if all conditions are met and the Linear, Conv3D, etc. files have a new argument for the cuda_malloc_heap_size.
The logic behind the device's programming has changed slightly to represent the way devices are programmed in our lab in practice. The amplitude of the voltage used to program the devices increases (or decreases) by 0.02 each time the desired conductance is not achieved. It could be good to implement in a future release a variable to change the nominal value of this increment. In the new routine, the variable force_adjustment_pos_voltage_threshold represents the maximum voltage that the incrementation of 0.02V can lead to (voltage will always be lower than this even with incrementation) and the force_adjustment_neg_voltage_threshold serves the same purpose for negative voltages .
For the networks I tested on, I did not manage to get to the end of the execution of the naive_program.py routine, so I do not have any estimate for the speedup it brings. Here are some execution times with respect to the size of the network (note that for the following networks, all tiles are initially randomized so every ReRAM needs to be programmed):
Patched Linear(in_features=100, out_features=50, bias=True) -> bh.Linear(in_features=100, out_features=50, bias=True)
Patched Linear(in_features=50, out_features=10, bias=True) -> bh.Linear(in_features=50, out_features=10, bias=True)
time = 123.00254679999999
Patched Linear(in_features=100, out_features=30, bias=True) -> bh.Linear(in_features=100, out_features=30, bias=True)
Patched Linear(in_features=30, out_features=10, bias=True) -> bh.Linear(in_features=30, out_features=10, bias=True)
time = 113.69156190000001
Patched Linear(in_features=25, out_features=30, bias=True) -> bh.Linear(in_features=25, out_features=30, bias=True)
Patched Linear(in_features=30, out_features=10, bias=True) -> bh.Linear(in_features=30, out_features=10, bias=True)
time = 110.4159673
I would like next to commit some plotting utility files next (mostly heatmaps comparing programming of devices before and after the programming routine) but I do not know where in memtorch's architecture this would be most appropriate. It would probably be good to add a section to the tutorial to guide users with this new functionality if plots are added and I could take care of this if you wish.
The version.py and setup.py files are updated by accident and I cannot revert them to the original form because of an error caused by black and isort after I have setup my environment using the suggested pre-commit commands. Please ignore them. I can create a new pull request if necessary