-
Notifications
You must be signed in to change notification settings - Fork 123
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
nrnbbcore_write code re-organisation #722
Conversation
I think that this refactoring is a good time to replace |
Noted. @alkino thank you for the review, this PR is however just about re-organising the code. Will tackle those when refactoring is of interest. Just made a few changes, will tag those so you can have a look. |
ffc3b53
to
7be0e38
Compare
Used this to have a sort of integration test, outputs are the same wrt |
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 PR is almost good for merge.
I didn't comment on the code style aspects as we want to limit this PR for code re-organisation and not changing the implementation.
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 is too much to pore over in detail. I trust things are just being copied to other places and enough include stuff is added to make everything consistent. It is a transition pull request to be extensively modified later. I wish #717 had made it in before this. When @pramodk approves, in it goes, unless we prefer to keep in different branch that receives pull requests til it is mature or nicely polished.
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.
as most of the changes are just re-organisation of code without any change is logic, I am also OK to merge this PR sooner. (after Alex give green signal)
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 triggering CI at BBP server as a sanity check. Will report is here once those tests passes.
Simulation CI job on BBP server is here.
I am seeing below error:
I wonder if this behaviour was introduced in master branch itself. I started new CI job for master branch of NEURON. |
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.
The issue I described in previous comment comes from the older commit in the master branch and we will handle that separately.
Discussed with Robert on slack and we will go ahead with the merge.
This PR targets
nrnbbcore_write
code re-organisation (and a few minor updates).It represents a first step for its refactoring.