nest / nest-gpu

NEST GPU
https://nest-gpu.readthedocs.io
GNU General Public License v2.0
19 stars 12 forks source link

Add placeholder comments for NESTML generated files #65

Closed pnbabu closed 1 year ago

pnbabu commented 1 year ago

The placeholder comments are used as a key to place the .h and .cu neuron model files generated from NESTML.

JoseJVS commented 1 year ago

Hi @pnbabu, could you explain your reasoning behind having the placeholders pushed into the main branch? From my understanding, these placeholders serve as a guide to manually write the generated NESTML model sources into the CMake files and this is solely for development purposes as currently we don't have the dynamic module loading system that NEST does. If this is correct then I don't think it is appropriate to have these placeholders in the main branch as we meant for this branch to be solely used for release code (similarly to how it is handled in the NEST repo), hence I would think it is enough to have it in your fork. If you either way feel the need to have these placeholders here, then I propose that we create a new branch in this repo based on your nestml_placeholder branch, however you won't have push rights to it and would still need to go through pull requests. Of course, if you have any other ideas we welcome the discussion :+1:

pnbabu commented 1 year ago

@JoseJVS The main reasoning for this was to have the placeholders for the NESTML-generated code be placed in the right positions. I was unaware of the timeline for developing the dynamic module loading system and thought it is better to have these in the codebase, eventually helping the CI.

However, as you suggest, maybe the master branch is not the right place for it. I can keep the changes in my fork right now and create a branch in the future when we see a need, specifically for the CI.

JoseJVS commented 1 year ago

I see, it certainly makes total sense to have the placeholders for the CI; as for the dynamic module loading system, hopefully it will be done during summer but we have yet to see how this goes, so if the placeholders help the CI, these should be clearly defined somewhere. However, I would still disagree with putting the placeholders in the main branch for the reasons I previously stated. If it is more convenient for the CI to have a branch in the NEST GPU repo with the placeholders then we can of course create this branch for you, otherwise I would suggest to change this PR to draft state and leave it here for documentation purposes until the dynamic module loading system is ready.

pnbabu commented 1 year ago

@JoseJVS I agree. I can close this PR as the changes are there in my fork. We can create a new branch and I can move these changes there when we have things moving with the CI.