Conversation
- move information from setup.py to pyproject.toml - optional dependencies for tooling used
- test 1 - no result obtained at all - test 2 - wrong growth given
- to be updated using loc operator
|
With pandas three we will need to use /Users/heweb/Documents/repos/croissance/croissance/estimation/smoothing/segments.py:19: FutureWarning: The behavior of obj[i:j] with a float-dtype index is deprecated. In a future version, this will be treated as positional instead of label-based. For label-based slicing, use obj.loc[i:j] instead
window = series[i : i + size * increment]
/Users/heweb/Documents/repos/croissance/croissance/estimation/smoothing/segments.py:62: FutureWarning: The behavior of obj[i:j] with a float-dtype index is deprecated. In a future version, this will be treated as positional instead of label-based. For label-based slicing, use obj.loc[i:j] instead
window = series[start:end] |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the project's build and CI/CD infrastructure by migrating from Travis CI to GitHub Actions, converting package metadata from setup.py to pyproject.toml, and refactoring tests to run independently without PDFWriter dependencies.
Changes:
- Adds comprehensive GitHub Actions workflow with formatting checks (black, isort), linting (ruff), testing across Python versions, and automated PyPI publishing
- Migrates package configuration from setup.py to pyproject.toml with setuptools_scm for version management
- Splits monolithic test_process_curve_basic into three independent test functions (test_process_curve_basic0/1/2) and separates PDFWriter testing
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/cicd.yml | Adds new CI/CD pipeline with format, lint, test, build, and publish jobs |
| .github/workflows/README.md | Documents the CI/CD workflow structure and purpose |
| pyproject.toml | Migrates all package metadata, dependencies, and tooling configuration from setup.py |
| setup.py | Strips down to minimal setup() call with only find_packages() |
| tests/api_test.py | Refactors test_process_curve_basic into separate test functions and isolated PDFWriter test |
| .travis.yml | Removes deprecated Travis CI configuration |
| .gitignore | Expands with modern Python development patterns |
| croissance/*.py | Applies consistent import ordering per isort/black standards |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if: startsWith(github.ref, 'refs/heads/main') || startsWith(github.ref, 'refs/tags') | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
The build_source_dist and publish jobs use actions/checkout@v4 without specifying fetch-depth. When using setuptools_scm for version management, you may need full git history to correctly determine the version. Consider adding 'fetch-depth: 0' to the checkout action to ensure setuptools_scm has access to all tags and commit history.
| - uses: actions/checkout@v4 | |
| - uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 |
|
|
||
| - uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: "3.10" |
There was a problem hiding this comment.
Python version "3.10" should be quoted as a string. Without quotes, YAML interprets this as the float 3.1, which will cause the setup-python action to fail. Change to "3.10".
| runs-on: macos-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.11", "3.12", "3.13"] |
There was a problem hiding this comment.
The test matrix only includes Python versions 3.11, 3.12, and 3.13, but pyproject.toml specifies requires-python = ">=3.9". This means Python 3.9 and 3.10 are supported but not tested. Either update the test matrix to include ["3.9", "3.10", "3.11", "3.12", "3.13"] or adjust the requires-python specification in pyproject.toml.
| "matplotlib>=1.4.3", | ||
| "numpy>=1.9.1", | ||
| "pandas>=0.18.0,<3.0.0", | ||
| "scipy>=0.14.0", |
There was a problem hiding this comment.
The dependency versions appear very old (matplotlib>=1.4.3 from 2015, numpy>=1.9.1 from 2014, pandas>=0.18.0 from 2016, scipy>=0.14.0 from 2014). These ancient versions may not be compatible with modern Python versions (3.9+). Consider updating the minimum versions to those compatible with Python 3.9+, or verify that these old versions actually work with the specified Python versions.
| "matplotlib>=1.4.3", | |
| "numpy>=1.9.1", | |
| "pandas>=0.18.0,<3.0.0", | |
| "scipy>=0.14.0", | |
| "matplotlib>=3.3.0", | |
| "numpy>=1.19.2", | |
| "pandas>=1.2.0,<3.0.0", | |
| "scipy>=1.5.0", |
| result = process_curve(curve, constrain_n0=True, n0=0.0) | ||
|
|
||
| # self.assertAlmostEqual(mu, slope, 6, msg='growth rate (mu)={}'.format(mu)) | ||
| # self.assertAlmostEqual(mu, slope, 6, msg="growth rate (mu)={}".format(mu)) |
There was a problem hiding this comment.
The comment references "growth rate (mu)=" but uses the old Python 2 style .format() when there are no actual assertions or usage of this commented-out code. Since this is a commented line left from old unittest code (self.assertAlmostEqual), consider removing it entirely as it provides no value in its current state.
| assert result.growth_phases[0].SNR > 1000 | ||
| result = process_curve(curve, constrain_n0=True, n0=0.0) | ||
|
|
||
| # self.assertAlmostEqual(mu, slope, 6, msg="growth rate (mu)={}".format(mu)) |
There was a problem hiding this comment.
The comment references "growth rate (mu)=" but uses the old Python 2 style .format() when there are no actual assertions or usage of this commented-out code. Since this is a commented line left from old unittest code (self.assertAlmostEqual), consider removing it entirely as it provides no value in its current state.
| def test_process_curve_basic0(): | ||
| # with PDFWriter(Path("test.basic.pdf")) as doc: | ||
| mu = 0.5 | ||
| pph = 4.0 | ||
| curve = pandas.Series( | ||
| data=[numpy.exp(mu * i / pph) for i in range(100)], | ||
| index=[i / pph for i in range(100)], | ||
| ) | ||
|
|
||
| result = process_curve(curve, constrain_n0=True, n0=0.0) | ||
| doc.write("#2 n0=0", result) | ||
| print(result.growth_phases) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-2) | ||
| assert 0.0 == approx(result.growth_phases[0].n0, 1e-6) | ||
| assert result.growth_phases[0].SNR > 1000 | ||
|
|
||
| result = process_curve(curve, constrain_n0=False) | ||
| doc.write("#2 n0=auto", result) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-2) | ||
| assert -0.25 < result.growth_phases[0].n0 < 0.25 | ||
| assert result.growth_phases[0].SNR > 1000 | ||
| result = process_curve(curve, constrain_n0=True, n0=0.0) | ||
|
|
||
| # self.assertAlmostEqual(mu, slope, 6, msg="growth rate (mu)={}".format(mu)) | ||
|
|
||
| # doc.write("#0 n0=1", result) | ||
| # print(result.growth_phases) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-2) | ||
| assert 0.0 == approx(result.growth_phases[0].n0, 1e-3) | ||
| assert result.growth_phases[0].SNR > 1000 | ||
|
|
||
| result = process_curve(curve, constrain_n0=False) | ||
| # doc.write("#0 n0=auto", result) | ||
| # print(result.growth_phases) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-2) | ||
| assert -0.25 < result.growth_phases[0].n0 < 0.25 | ||
| assert result.growth_phases[0].SNR > 1000 | ||
|
|
||
|
|
||
| def test_process_curve_basic1(): | ||
|
|
||
| mu = 0.5 | ||
| pph = 4.0 | ||
|
|
||
| curve = pandas.Series( | ||
| data=( | ||
| [1.0] * 5 | ||
| + [numpy.exp(mu * i / pph) for i in range(25)] | ||
| + [numpy.exp(mu * 24 / pph)] * 20 | ||
| ), | ||
| index=([i / pph for i in range(50)]), | ||
| ) | ||
|
|
||
| result = process_curve(curve, constrain_n0=True, n0=0.0) | ||
| # doc.write("#1 n0=0", result) | ||
| # print(result.growth_phases) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-2) | ||
| assert 0.0 == approx(result.growth_phases[0].n0, 1e-3) | ||
| assert result.growth_phases[0].SNR > 1000 | ||
|
|
||
| result = process_curve(curve, constrain_n0=False) | ||
| # doc.write("#1 n0=auto", result) | ||
| # print(result.growth_phases) | ||
| assert len(result.growth_phases) == 1 | ||
| assert mu == approx(result.growth_phases[0].slope, abs=1e-1) | ||
| assert -0.25 < result.growth_phases[0].n0 < 0.25 | ||
| assert result.growth_phases[0].SNR > 1000 | ||
|
|
||
|
|
||
| def test_process_curve_basic2(): |
There was a problem hiding this comment.
Test function names like test_process_curve_basic0, test_process_curve_basic1, and test_process_curve_basic2 are not descriptive. Consider using more meaningful names that describe what each test variant is testing, such as test_process_curve_exponential_only, test_process_curve_with_lag_and_stationary, and test_process_curve_with_linear_ramp. This would make test failures easier to understand.
|
|
||
| build_source_dist: | ||
| name: Build source distribution | ||
| if: startsWith(github.ref, 'refs/heads/main') || startsWith(github.ref, 'refs/tags') |
There was a problem hiding this comment.
The condition uses startsWith(github.ref, 'refs/heads/main') which will trigger builds on every push to main, not just on tags. Given that setuptools_scm derives version from git tags, building from non-tag commits on main may result in development versions. If the intent is to only build releases, consider removing the 'refs/heads/main' condition and only keeping 'refs/tags'.
| if: startsWith(github.ref, 'refs/heads/main') || startsWith(github.ref, 'refs/tags') | |
| if: startsWith(github.ref, 'refs/tags') |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.