Remove unused cylindrical battery geometry support

[vidipsingh]

Description

This PR removes the handling for cylindrical battery geometry from the battery_geometry function, as it is not used anywhere within the repository and was initially meant for future thermal model extensions.

Fixes: #4856

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

• No style issues: nox -s pre-commit
• All tests pass: nox -s tests
• The documentation builds: nox -s doctests
• Code is commented for hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[prady0t]

If you plan to remove this, you will also need to modify the associated tests. Before opening a PR, run the tests via nox -s unit to get the error log (if any) locally and try to remove them.

[vidipsingh]

If you plan to remove this, you will also need to modify the associated tests. Before opening a PR, run the tests via nox -s unit to get the error log (if any) locally and try to remove them.

Thanks for the heads-up! I'll modify the associated tests and run nox -s unit to check for any errors. I'll update the PR accordingly after resolving them.

[vidipsingh]

Hi @prady0t, I've updated the tests related to the cylindrical geometry removal and ran nox -s unit to confirm everything passes successfully. Could you kindly review the changes and let me know if there's anything else I need to address?

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.71%. Comparing base (8f91615) to head (ef3b104). Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4875      +/-   ##
===========================================
- Coverage    98.71%   98.71%   -0.01%     
===========================================
  Files          304      304              
  Lines        23509    23503       -6     
===========================================
- Hits         23207    23200       -7     
- Misses         302      303       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[prady0t]

Test wise it looks fine.

[vidipsingh]

Test wise it looks fine.

One test was failing, so I removed the test_cylindrical_div_convergence_quadratic test. After that, there are no errors when running nox -s unit.

[vidipsingh]

The "cylindrical" finite volume tests are different from the "cylindrical" geometry, I don't know why those tests faield but they should not be removed

Got it! I’ll revert the changes then. Thanks for clarifying!

[vidipsingh]

looks good , thanks!

Sure, anytime! Glad it's good to go.

[kratman]

@vidipsingh It looks like tests are still failing here

[vidipsingh]

@vidipsingh It looks like tests are still failing here

@kratman Thanks for pointing out the failing tests. When I ran the nox -s unit test command locally, I encountered the following errors:

nox -s unit Output:

============================================================================== short test summary info ===============================================================================
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:19: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:23: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:27: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:31: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:35: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:39: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:43: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:47: Test currently not implemented
SKIPPED [1] tests/unit/test_models/test_full_battery_models/test_lithium_ion/test_newman_tobias.py:51: Test currently not implemented
SKIPPED [1] tests/unit/test_solvers/test_idaklu_jax.py:118: JAX is available
FAILED tests/unit/test_spatial_methods/test_finite_volume/test_grad_div_shapes.py::TestFiniteVolumeGradDiv::test_cylindrical_grad_div_shapes_Dirichlet_bcs - pybamm.expression_tree.exceptions.GeometryError: Invalid form factor 'cylindrical' (should be 'pouch')
FAILED tests/unit/test_spatial_methods/test_finite_volume/test_grad_div_shapes.py::TestFiniteVolumeGradDiv::test_cylindrical_grad_div_shapes_Neumann_bcs - pybamm.expression_tree.exceptions.GeometryError: Invalid form factor 'cylindrical' (should be 'pouch')
FAILED tests/unit/test_spatial_methods/test_finite_volume/test_integration.py::TestFiniteVolumeIntegration::test_definite_integral - pybamm.expression_tree.exceptions.GeometryError: Invalid form factor 'cylindrical' (should be 'pouch')
===================================================== 3 failed, 1828 passed, 10 skipped, 38 subtests passed in 312.08s (0:05:12) =====================================================
nox > Command python -m pytest -m unit failed with exit code 1
nox > Session unit failed.

Specifically, the errors are occurring in the following test functions:

• test_cylindrical_grad_div_shapes_Dirichlet_bcs
• test_cylindrical_grad_div_shapes_Neumann_bcs
• test_definite_integral

Previously, when @prady0t mentioned that the test cases were failing, I removed these functions to resolve the issue. However, @valentinsulzer pointed out that these cylindrical finite volume tests are different from the cylindrical geometry changes in this PR and should not be removed. Based on that feedback, I reverted those changes in commit 4c9b3e6.

Now, the same test errors reappeared with an Invalid form factor 'cylindrical' exception, suggesting the tests still expect a removed cylindrical form factor. Could you please advise on how I should resolve these test errors while keeping the finite volume tests intact as suggested by @valentinsulzer?

[vidipsingh]

@vidipsingh It looks like tests are still failing here

@kratman Thanks for pointing out the failing tests. When I ran the nox -s unit test command locally, I encountered the following errors:

nox -s unit Output: Specifically, the errors are occurring in the following test functions:

• test_cylindrical_grad_div_shapes_Dirichlet_bcs
• test_cylindrical_grad_div_shapes_Neumann_bcs
• test_definite_integral

Previously, when @prady0t mentioned that the test cases were failing, I removed these functions to resolve the issue. However, @valentinsulzer pointed out that these cylindrical finite volume tests are different from the cylindrical geometry changes in this PR and should not be removed. Based on that feedback, I reverted those changes in commit 4c9b3e6.

Now, the same test errors reappeared with an Invalid form factor 'cylindrical' exception, suggesting the tests still expect a removed cylindrical form factor. Could you please advise on how I should resolve these test errors while keeping the finite volume tests intact as suggested by @valentinsulzer?

@kratman, just following up on this—could you please provide guidance on resolving these test errors while ensuring the finite volume tests remain intact, as suggested by @valentinsulzer?

[vidipsingh]

Hi @kratman @valentinsulzer, Just wanted to follow up on this PR and check if there are any changes needed from my side. It would be great if you could review it and let me know if any changes are required.

[kratman]

@vidipsingh You still need to find a way to fix the tests if you want to continue on this PR

[kratman]

@vidipsingh Are you still working on this?

[vidipsingh]

@vidipsingh Are you still working on this?

Hi @kratman, sorry for being inactive. Yes, I’ll look into fixing the test issues.