Fix edge case in LaminarCurrentSourceDensity, use Poetry for builds, Python >= 3.10#197
Open
Fix edge case in LaminarCurrentSourceDensity, use Poetry for builds, Python >= 3.10#197
Conversation
There was a problem hiding this comment.
Pull request overview
Updates LFPykit’s packaging and CI to modern Python/PEP 517 tooling while fixing an edge case in LaminarCurrentSourceDensity that caused non-zero CSD sums in certain geometries (Issue #196).
Changes:
- Fix
LaminarCurrentSourceDensity.get_transformation_matrix()fraction calculation when radius constraints are non-binding (z-plane crossing vs r–z intersection). - Add Poetry-based
pyproject.toml, remove legacysetup.py/setup.cfg, and bump version to0.6.0with Python>=3.10. - Extend CI matrix to newer Python versions and add regression tests covering Issue #196.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| setup.py | Removed legacy setuptools build script in favor of PEP 517/Poetry. |
| setup.cfg | Removed legacy setuptools metadata config. |
| pyproject.toml | Introduces Poetry build configuration and dependency declarations. |
| lfpykit/version.py | Version bump to 0.6.0. |
| lfpykit/tests/test_module.py | Adds regression tests for Laminar CSD Issue #196; renames test ids for consistency. |
| lfpykit/models.py | Fixes Laminar CSD volume fraction calculation and simplifies _PrPz segment-hit logic. |
| .gitignore | Ignores macOS archive artifacts and an additional model directory. |
| .github/workflows/python-app.yml | Updates CI Python version matrix to 3.10–3.14. |
| .github/workflows/flake8.yml | Updates lint job Python version to 3.12. |
| .github/workflows/coveralls.yml | Updates coverage job Python version to 3.12. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…eferencing in extras Agent-Logs-Url: https://github.com/LFPy/LFPykit/sessions/6172e06c-8d32-44a4-9019-3a15086bbb76 Co-authored-by: espenhgn <2492641+espenhgn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Updates LFPykit's packaging and CI to modern Python/PEP 517 tooling while fixing an edge case in
LaminarCurrentSourceDensitythat caused non-zero CSD sums in certain geometries.Changes:
LaminarCurrentSourceDensity.get_transformation_matrix()fraction calculation when radius constraints are non-binding (z-plane crossing vs r–z intersection).pyproject.toml, remove legacysetup.py/setup.cfg, and bump version to0.6.0with Python>=3.10.[tool.poetry.extras]: declarepytest,sympy,sphinx,numpydoc,sphinx_rtd_theme, andrecommonmarkas optional dependencies in[tool.poetry.dependencies]so thatpoetry install -E testsandpoetry install -E docswork correctly, mirroring the oldextras_requirebehavior fromsetup.py.Related Issue
Motivation and Context
The
LaminarCurrentSourceDensityclass produced non-zero CSD sums in certain geometries due to an incorrect fraction calculation. The legacysetup.py-based packaging was replaced with a Poetry/PEP 517 setup for modern tooling support. The initial Poetry extras configuration was missing the required optional dependency declarations, preventing extras from being installable.How Has This Been Tested
LaminarCurrentSourceDensityedge case.[tool.poetry.extras]are declared as optional in[tool.poetry.dependencies].Types of changes
Checklist
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.