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

Refactor neuron.coreneuron for Python 3.10.x #1528

Merged
merged 3 commits into from
Nov 11, 2021
Merged

Conversation

olupton
Copy link
Collaborator

@olupton olupton commented Nov 10, 2021

This fixes some Python 3.10.0 incompatibilities, as partly discussed in #1525.

The main changes are:

  • Reorganise neuron.coreneuron to be a class instance with getter/setter methods, so that properties like coreneuron.enable and coreneuron.cell_permute always have consistent values of consistent types. This stops us from relying on implicit float -> int conversions in the Python C API, as described in NEURON build + tests do not pass on Fedora latest #1525.
  • Use collections.abc.Callable instead of collections.Callable, which is deprecated.
  • In test/coreneuron/test_datareturn.py compare the two coreneuron.cell_permute values that are valid given coreneuron.gpu, instead of always trying to compare 0 and 1.

Closes #1525.

I tested this on BB5 + GPU (with Python 3.8) and on Fedora 35 in Docker. I still saw a couple of failures locally, but I think these are unrelated and that something is slightly broken in my macOS + Docker + Fedora 35 + MPI environment.

@olupton
Copy link
Collaborator Author

olupton commented Nov 10, 2021

https://github.com/neuronsimulator/nrn-build-ci/runs/4163851845 shows this fixes test execution on fedora:latest, although the CI run still fails because we do not build nightly wheels for Python 3.10, it seems.

@olupton
Copy link
Collaborator Author

olupton commented Nov 10, 2021

The fedora:latest job passed when I tried running it on this branch: https://github.com/neuronsimulator/nrn-build-ci/runs/4164091280.

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

LGTM. I guess we just need to wait for #1524 to fix #1522 so that the windows builds work again.

Copy link
Member

@alkino alkino left a comment

Choose a reason for hiding this comment

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

Nice cleaning

@pramodk pramodk merged commit 3d0e2b5 into master Nov 11, 2021
@pramodk pramodk deleted the olupton/coreneuron-python branch November 11, 2021 15:57
@pramodk
Copy link
Member

pramodk commented Nov 11, 2021

Ignoring Windows CI as that's need to be handled separately anyway.

alexsavulescu pushed a commit that referenced this pull request Jan 28, 2022
* Add getters/setters to neuron.coreneuron 'module'.
* Collections.Callable is deprecated.
* Code cleanup
@alexsavulescu alexsavulescu mentioned this pull request Mar 22, 2022
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NEURON build + tests do not pass on Fedora latest
4 participants