Skip to content
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

Unifying system for applying substitutions #256

Merged
merged 12 commits into from
Sep 5, 2019
Merged

Conversation

neworderofjamie
Copy link
Contributor

@neworderofjamie neworderofjamie commented Sep 3, 2019

I'm afraid this is another building block type feature upon which more exciting things can be built. Basically, in GeNN 4 I added the Substitutions class which managed 'individual' substitutions of stuff like "$(id)". However there was still name_substitution and value_substitution calls going on all over the place for model parameters, variables etc and these were implemented with some particularly gnarly C++ I added early in my GeNN career!

Having a single unified system for substituting stuff not only tidies stuff up but, in future, could allow us to do stuff like add reference counts meaning that things like #232 #47 and #55 could be addressed in a nicer way that just adding increasingly-complex if clauses around them

@codecov
Copy link

codecov bot commented Sep 3, 2019

Codecov Report

Merging #256 into master will increase coverage by 18.99%.
The diff coverage is 87.09%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #256       +/-   ##
===========================================
+ Coverage   41.07%   60.07%   +18.99%     
===========================================
  Files          46       47        +1     
  Lines        6283     6335       +52     
===========================================
+ Hits         2581     3806     +1225     
+ Misses       3702     2529     -1173
Impacted Files Coverage Δ
include/genn/genn/code_generator/codeGenUtils.h 95.23% <ø> (-0.22%) ⬇️
src/genn/genn/modelSpec.cc 58.06% <100%> (+2.98%) ⬆️
src/genn/genn/code_generator/codeGenUtils.cc 75.25% <100%> (+2.97%) ⬆️
src/genn/genn/code_generator/generateInit.cc 60.97% <73.33%> (-0.92%) ⬇️
include/genn/genn/code_generator/substitutions.h 85% <80.64%> (+12.08%) ⬆️
...c/genn/genn/code_generator/generateNeuronUpdate.cc 61.81% <88.33%> (+1.49%) ⬆️
.../genn/genn/code_generator/generateSynapseUpdate.cc 63.54% <90.32%> (-8.03%) ⬇️
include/genn/genn/neuronGroup.h 69.23% <0%> (-3.85%) ⬇️
src/genn/genn/code_generator/generateRunner.cc 52.39% <0%> (-2.59%) ⬇️
src/genn/backends/single_threaded_cpu/backend.cc 87.41% <0%> (-1.52%) ⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 86c8a18...850b120. Read the comment docs.

Copy link
Member

@tnowotny tnowotny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the right thing to do. I have not looked in to the details of how you have implemented it but it looks like an entirely reasonable syntax.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants