Python wrapper #202
Python wrapper #202
Conversation
…on and NNmodel::addSynapsePopulation, adapt generateALL main() for swig
…offers an end-to-end solution now (define simulation, build it, run, and get results as numpy array)
…es via numpy arrays; update GeNNModel.py wrapper to account for these changes
…without building it
The helper functions to create Param and VarValues are now in their respective classes added as swig extensions instead of being standalone.
introduce a better way to determine whether the model is Custom; change Values type to dict instead of list. other minor changes caused by ongoing integration with PyNN
…e extroGlobalParams on GPU to NNmodel class
…pygenn population (grougs)s to their own classes; minor bug fixes and improvements
make assingment of the numpy arrays to C pointers more generic fix current spikes extraction
rework swig interface generator completely, many SWIG interface files are now autogenerated; rename libgenn to pygenn to distinguish it from pure C++ libgenn; add virtual destructor to Snippet::Base to avoid memory leaks when using SWIG's directors feature;
add support for variable initialization using InitVarSnippets; add custom initVarSnippet;
lib/include/generateALL.h
Outdated
| @@ -51,3 +51,9 @@ void chooseDevice(NNmodel &model, //!< the nn model we are generating code | |||
| const string &path, //!< path the generated code will be deposited | |||
| int localHostID); //!< ID of local host | |||
| #endif | |||
|
|
|||
| #ifdef SWIG | |||
There was a problem hiding this comment.
This seems a bit messy. I think it would be much nicer if the stuff you've added in init_cuda_mpi could go in the entry-point code for your SWIG module (I'm guessing that SWIG modules like native python modules have this) and finalize_model_runner_generation could go into the SWIG module as well.
There was a problem hiding this comment.
This makes a lot of sense. I do not like the current solution either 😅
lib/include/modelSpec.h
Outdated
| @@ -233,9 +233,11 @@ class NNmodel | |||
| const typename NeuronModel::VarValues &varInitialisers, | |||
| int hostID = 0, int deviceID = 0) | |||
| { | |||
| #ifndef SWIG | |||
There was a problem hiding this comment.
Likewise, if you had some entry-point code for your SWIG module you could just call initGeNN from there and get rid of the need for this (I realise initGeNN is pretty crap and it is going to be deprecated but still)
There was a problem hiding this comment.
Initially I had initGeNN wrapped, but after careful examination I realized that all it does is initializing legacy models and as you said will most likely be deprecated. I thought it was a good idea not to include it in SWIG, but I can revert it back.
There was a problem hiding this comment.
I think it's better to include it (or at least just add a hack that sets GeNNReady= 1) than to scatter #ifdefs everywhere
lib/swig/SharedLibraryModel.h
Outdated
| // Retrive symbols to pull/push from/to the device from shared model | ||
| // and store them in a map for fast lookup | ||
| void initIO( const std::string &popName, | ||
| const std::array<bool, 5> &availableDTypes ) |
There was a problem hiding this comment.
It is super not-important but you could potentially use a std::bitset<5> here
lib/swig/SharedLibraryModel.h
Outdated
| VoidFunction m_StepTimeCPU; | ||
| VoidFunction m_ExitGeNN; | ||
|
|
||
| PopIOMap populationsIO; |
There was a problem hiding this comment.
This should be m_PopulationsIO in keeping with the general naming convention.
lib/GNUmakefile
Outdated
| @@ -22,6 +22,9 @@ | |||
| # Include makefile which builds libgenn | |||
| include $(GENN_PATH)/lib/GNUMakefileLibGeNN | |||
|
|
|||
| # Include makefile which builds python wrapper | |||
| include $(GENN_PATH)/lib/GNUMakefilePyGeNN | |||
There was a problem hiding this comment.
I think it would be nicer if GNUMakefilePyNN included GNUMakefileLibGeNN rather than the other way around
There was a problem hiding this comment.
GNUMakefilePyNN implicitly includes GNUMakefileLibGeNN by coming after it. However many variables are not very helpful because they are libgenn specific. For example, LINK_FLAGS contain -lgenn which is not needed and not built if the target is pygenn.
There was a problem hiding this comment.
I know, but the point of GNUMakefile to to build generateAll (which admittedly may not be clear from it's current name...) and generateAll links to libgenn so its makefile includes GNUMakefileLibGeNN. Likewise the point of GNUMakefilePyGeNN is to build PyGeNN so it should include GNUMakefileLibGeNN rather than be part of GNUMakefile
There was a problem hiding this comment.
How come pygenn doesn't link against libgenn or is that just not how SWIG works?
lib/GNUMakefilePyGeNN
Outdated
| PYGENN_LINK_FLAGS +=$(shell mpiCC -showme:link) | ||
| endif | ||
|
|
||
| PYGENN_PIC_OBJ :=global modelSpec neuronGroup synapseGroup currentSource initVarSnippet utils codeStream codeGenUtils sparseUtils neuronModels postSynapseModels synapseModels newNeuronModels newPostsynapticModels newWeightUpdateModels currentSourceModels standardSubstitutions standardGeneratedSections generateALL generateCPU generateInit generateKernels generateMPI generateRunner |
There was a problem hiding this comment.
If you included GNUMakefileLibGeNN as I suggest below, I'm fairly sure this is just a copy of LIBGENN_OBJ. It would be REALLY good if we didn't have to maintain that list in more places (having it in the Windows and *nix build system is bad enough)
There was a problem hiding this comment.
it is a copy of original LIBGENN_OBJ however the LIBGENN_OBJ are already fully qualified paths by this point.
There was a problem hiding this comment.
Yeah I'd be more than happy if you made GNUMakefileLibGeNN more flexible i.e. by keeping the unqualified paths separate.
There was a problem hiding this comment.
Another issue with this makefile structure that, when you try and do something else with GeNN and you it can't find your python/numpy source, you can't run genn-build-model.sh!
lib/GNUMakefilePyGeNN
Outdated
| cp $(SWIG_PATH)/GeNNModel.py $(PYGENN_PATH)/GeNNModel.py | ||
| cp $(SWIG_PATH)/GeNNGroups.py $(PYGENN_PATH)/GeNNGroups.py | ||
|
|
||
| $(PYGENN_PATH)/_StlContainers.so: $(PYGENN_PATH)/stl_containers_wrap.cc |
There was a problem hiding this comment.
It would be really good if (after you include GNUMakefileLibGeNN 😄) you used the LIBGENN_PREFIX to make a unique object directory for each variant of the modules. Otherwise it's very easy to get into a mess of incompatible bits of CPU_ONLY and normal versions mixed together.
There was a problem hiding this comment.
I will try to adapt both make files to minimize redundancy
lib/swig/customParamValues.h
Outdated
| class ParamValues | ||
| { | ||
| public: | ||
| template<typename T> |
There was a problem hiding this comment.
How come this is templated?
There was a problem hiding this comment.
Well, it is just a Snippet::ValueBase with a relaxed constructor... And yes, the template here is absolutely unnecessary 😅
lib/swig/customParamValues.h
Outdated
| @@ -0,0 +1,37 @@ | |||
| #ifndef CUSTOMPARAMVALUES_H | |||
There was a problem hiding this comment.
We're generally trying to switch to using #pragma once rather than old style include guards
There was a problem hiding this comment.
I wasn't aware of #pragma once!
lib/swig/generate_swig_config.py
Outdated
| @@ -0,0 +1,538 @@ | |||
| import sys | |||
There was a problem hiding this comment.
What does this file do? It needs a lot of commenting!
There was a problem hiding this comment.
it generates interface files for SWIG. Some interfaces require many templates specializations and the interfaces for models look alike. So this file does all the copy-paste work 😃 Additionally it reads the models files and creates template specializations for them, so when a new model is added to GeNN, it will wrap it without any additional work.
There was a problem hiding this comment.
It would also be really nice if this stuff generated pydoc help - especially as someone who doesn't know how it works help(pygenn.NNmodel.addNeuronPopulation) gives me no clues as to what parameters are
There was a problem hiding this comment.
actually everything is supposed to work via GeNNModel class
There was a problem hiding this comment.
that explains why that wasn't working then 😄
lib/GNUMakefilePyGeNN
Outdated
| SWIG_PATH :=$(GENN_PATH)/lib/swig | ||
| PYGENN_PATH :=$(GENN_PATH)/lib/pygenn | ||
| PYGENN_SWIG_FILE_PATH :=$(GENN_PATH)/lib/$(SWIG_MODULE_NAME).i | ||
| PYTHON_INCLUDE ?=$(shell python-config --includes) |
There was a problem hiding this comment.
For me, this returns: -I/usr/include/python2.7 -I/usr/include/x86_64-linux-gnu/python2.7 so when you do -I $(PYTHON_INCLUDE) lower down it fails
There was a problem hiding this comment.
(it builds if I override it to /usr/include/python2.7 from the command line though)
There was a problem hiding this comment.
Right... Python 2.7 is still the standard on many systems, isn't it?
There was a problem hiding this comment.
Oh, so the issue is not the version, but the second include? I also have two, but they are the same on my system... I guess, taking only the first one should solve this.
There was a problem hiding this comment.
Hah indeed I do add -I... I wonder why it works 🤔
There was a problem hiding this comment.
By the way I cannot guarantee that Python 2.7 will work... Should we maybe restrict it to 3?
There was a problem hiding this comment.
I'm still figuring out how to actually do anything with it once it's imported, but it seems to work in Python 2.7. I don't really think restricting stuff to Python 3 is a legitimate option - a lot of people still use 2.7 (myself included)
There was a problem hiding this comment.
I will test it with 2.7.
|
Ahh is that what it is! But yes, ubuntu 16 LTS is gonna stay supported for
a good long time
…On Thu, 9 Aug 2018, 18:29 braintimeException, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/GNUMakefilePyGeNN
<#202 (comment)>:
> +## Institute: Center for Computational Neuroscience and Robotics
+## University of Sussex
+## Falmer, Brighton BN1 9QJ, UK
+##
+## email to: ***@***.***
+##
+## initial version: 2018-05-14
+##
+##--------------------------------------------------------------------------
+
+SWIG :=swig -c++ -python
+SWIG_MODULE_NAME :=pygenn
+SWIG_PATH :=$(GENN_PATH)/lib/swig
+PYGENN_PATH :=$(GENN_PATH)/lib/pygenn
+PYGENN_SWIG_FILE_PATH :=$(GENN_PATH)/lib/$(SWIG_MODULE_NAME).i
+PYTHON_INCLUDE ?=$(shell python-config --includes)
Right... Python 2.7 is still the standard on many systems, isn't it?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#202 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGeoGhsLRC5xlLTxfH6oAuxOhab0nSsAks5uPHGGgaJpZM4V18kh>
.
|
|
Yeah - python-config returns the -Is as well rather than just a path
, but you then add a - I yourself to the command line which breaks it
…On Thu, 9 Aug 2018, 18:40 braintimeException, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/GNUMakefilePyGeNN
<#202 (comment)>:
> +## Institute: Center for Computational Neuroscience and Robotics
+## University of Sussex
+## Falmer, Brighton BN1 9QJ, UK
+##
+## email to: ***@***.***
+##
+## initial version: 2018-05-14
+##
+##--------------------------------------------------------------------------
+
+SWIG :=swig -c++ -python
+SWIG_MODULE_NAME :=pygenn
+SWIG_PATH :=$(GENN_PATH)/lib/swig
+PYGENN_PATH :=$(GENN_PATH)/lib/pygenn
+PYGENN_SWIG_FILE_PATH :=$(GENN_PATH)/lib/$(SWIG_MODULE_NAME).i
+PYTHON_INCLUDE ?=$(shell python-config --includes)
Oh, so the issue is not the version, but the second include? I also have
two, but they are the same on my system... I guess, taking only the first
one should solve this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#202 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGeoGqLDVGNCgnmbtDQuCE7ilrvTJLDHks5uPHQdgaJpZM4V18kh>
.
|
lib/swig/GeNNGroups.py
Outdated
| """ | ||
| self.initRequired = False | ||
| if initVar is not None: | ||
| setRandomInit( initVar, values ) |
There was a problem hiding this comment.
Where is this function located and what does it do? Are InitVarSnippets implemented or should that be added to the Todo?
There was a problem hiding this comment.
InitVarSnippets were the last thing I implemented (actually just yesterday) and are not thoroughly tested, but they hopefully work. self.initRequired's purpose is to indicate that a list of values was provided and if it is true, these values will be pushed to device when the model is loaded.
There was a problem hiding this comment.
Cool that they're implemented! What I meant though is where is setRandomInit? I can't find it in the module...
There was a problem hiding this comment.
oops that's a typo. It must read self.setInitVar.
lib/swig/GeNNModel.py
Outdated
|
|
||
| for popName, popData in self.synapsePopulations.items(): | ||
| if popData.sparse: | ||
| popData.pop.setMaxConnections( popData.size ) |
There was a problem hiding this comment.
For sparse projections, max connections means (I realise it's badly named - sorry) the largest number of postsynaptic neurons any presynaptic neuron is connected to i.e. the longest length of a sparse synaptic matrix row - on that basis I'm not sure if this is correct.
There was a problem hiding this comment.
oh. I clearly misunderstood the name.
There was a problem hiding this comment.
No worries - that one is clearly Thomas's fault 😄 It's important to get that one right as it defines how many CUDA threads are used to simulate the synapses
… not working on mac
…g, installing etc
* All modules need to be in package * -py3 SWIG option needs to be given
…REQUIRED for Python 3
…is only REQUIRED for Python 3" This reverts commit d9153d8.
… it here or in PyNN GeNN
neworderofjamie
force-pushed
the
python_wrapper
branch
from
574a1d4
to
fb3a0e2
Compare
… when using it from Python
…er files to force recompile after the header has changed - Jamie, please check whether this makes sense.
…CDF of gamma distribution
|
agreed! |
# Conflicts: # version.txt
The python wrapper or pygenn is mostly ready.
What is supported:
GeNNModelPython helper class is the main proxy for model manipulation and simulation.NeuronModels,PostsynapticModels,WeightUpdateModels,CurrentSourceModelsandInitVarSnippets) can be defined directly in Python. There are alsocreateCustom*Classfunctions to simplify this process.addNeuronPopulation,addSynapsePopulationandaddCurrentSourcefunctions which closely resemble their GeNN counterparts.buildandloadinGeNNModel.numpyarray views which works as a pointer (or IS a pointer as far as I understand) to the underlying C-array. If you want to record, simply copy the contents of such array view elsewhere.How to run:
make -f GNUMakefilePyGeNN pygennpygennfolder to yourPYTHONPATHTODO by @neworderofjamie before merging