Adds `gradient_squared` method to `FiniteVolume` #4540

medha-14 · 2024-10-23T13:24:09Z

Description

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

New feature (non-breaking change which adds functionality)
Optimization (back-end change that speeds up the code)
Bug fix (non-breaking change which fixes an issue)

Key checklist:

No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
All tests pass: $ python run-tests.py --all (or $ nox -s tests)
The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

Code is commented, particularly in hard-to-understand areas
Tests added that prove fix is effective or that feature works

codecov · 2024-10-23T14:29:36Z

Codecov Report

Attention: Patch coverage is 7.14286% with 13 lines in your changes missing coverage. Please review.

Project coverage is 98.66%. Comparing base (298e306) to head (d92b43e).

Files with missing lines	Patch %	Lines
src/pybamm/spatial_methods/finite_volume.py	7.14%	13 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4540      +/-   ##
===========================================
- Coverage    98.71%   98.66%   -0.06%     
===========================================
  Files          304      304              
  Lines        23495    23509      +14     
===========================================
+ Hits         23193    23194       +1     
- Misses         302      315      +13

☔ View full report in Codecov by Sentry.
📢 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.

kratman · 2024-10-23T14:31:43Z

@medha-14 This PR should also add tests and use the gradient squared function where appropriate

medha-14 · 2024-10-28T10:59:57Z

I have written a test for the gradient_squared method . Could you please advise on where I should incorporate the use of the gradient_squared method ?

brosaplanella

Just some changes, mostly to improve coverages, but getting there!

src/pybamm/spatial_methods/finite_volume.py

agriyakhetarpal · 2024-11-07T10:14:56Z

src/pybamm/spatial_methods/finite_volume.py

+    def gradient_squared(self, symbol, discretised_symbol, boundary_conditions):
+        """
+        Computes the square of the gradient of a symbol.
+
+        Parameters
+        ----------
+        symbol : :class:`pybamm.Symbol`
+            The symbol for which to compute the gradient squared.
+        discretised_symbol : :class:`pybamm.Vector`
+            The discretised variable for which to compute the gradient squared.
+        boundary_conditions : dict
+            Boundary conditions for the symbol.
+
+        Returns
+        -------
+        float
+            The gradient squared of the symbol.
+        """
+        domain = symbol.domain
+        gradient_matrix = self.gradient_matrix(domain, symbol.domains)
+
+        # Compute gradient squared: (∇u)^2 = u^T (L^T L) u
+        gradient_squared_matrix = gradient_matrix.T @ gradient_matrix
+        gradient_squared_result = (
+            discretised_symbol.T @ gradient_squared_matrix @ discretised_symbol
+        )
+
+        return gradient_squared_result.item()


Here, boundary_conditions is mentioned in the function's signature, but it doesn't look like it's used in the logic of the function anywhere?

Thank you for the feedback. I've updated the gradient_squared function to correctly handle boundary conditions, and I've adjusted the tests accordingly to verify these changes.

agriyakhetarpal

Thanks, @medha-14!

src/pybamm/spatial_methods/finite_volume.py

agriyakhetarpal · 2024-11-10T23:07:44Z

I noticed that in the gradient_matrix method above the lines you touched, we could try building the gradient matrix directly in sparse format instead of converting to it later as a potential further improvement – by just pre-allocating all indices at once, and passing them to scipy.sparse.block_diag in favour of the current Kronecker product. Some part of the time there is spent in the array creation alongside the matrix construction, so maybe we can get a 2-4x improvement for larger matrices. Would you like to try that (not in this PR but in a separate one)? I'll be able to help, shall you need it.

medha-14 · 2024-11-11T11:25:51Z

I noticed that in the gradient_matrix method above the lines you touched, we could try building the gradient matrix directly in sparse format instead of converting to it later as a potential further improvement – by just pre-allocating all indices at once, and passing them to scipy.sparse.block_diag in favour of the current Kronecker product. Some part of the time there is spent in the array creation alongside the matrix construction, so maybe we can get a 2-4x improvement for larger matrices. Would you like to try that (not in this PR but in a separate one)? I'll be able to help, shall you need it.

@agriyakhetarpal, I would like to give this a go and explore the implementation further. I’d be grateful for any additional insights or recommendations you might have as I move forward.

medha-14 · 2024-12-11T10:28:48Z

Any recommendations on how to improve the tests I have written for this one? or any other things I can change?

agriyakhetarpal

Thanks, @medha-14! The code looks fine to me as long as coverage passes – I'm happy to approve it after that. I just have one additional comment.

agriyakhetarpal · 2025-02-10T00:08:35Z

tests/unit/test_spatial_methods/test_scikit_finite_element.py

+        np.testing.assert_array_almost_equal(grad_squared_result, inner_product)
+        np.testing.assert_array_less(0, grad_squared_result)


Could you please switch to np.testing.assert_allclose() here while we're at it? xref #4488

agriyakhetarpal · 2025-02-10T00:08:42Z

tests/unit/test_spatial_methods/test_scikit_finite_element.py

+        np.testing.assert_array_almost_equal(
+            grad_squared_result_dirichlet, inner_product
+        )
+        np.testing.assert_array_less(0, grad_squared_result_dirichlet)


Same as above

agriyakhetarpal · 2025-02-10T00:08:48Z

tests/unit/test_spatial_methods/test_scikit_finite_element.py

+        np.testing.assert_array_almost_equal(grad_squared_result_neumann, inner_product)
+
+        np.testing.assert_array_less(0, grad_squared_result_neumann)


Same as above

medha-14 · 2025-02-10T09:05:54Z

Thanks, @medha-14! The code looks fine to me as long as coverage passes – I'm happy to approve it after that. I just have one additional comment.

I will prioritize fixing the coverage as soon as possible.

…sing"

medha-14 requested review from martinjrobins, rtimms and valentinsulzer as code owners October 23, 2024 13:24

medha-14 changed the title ~~Adds gradient_squared method to FiniteVolume`~~ Adds gradient_squared method to FiniteVolume Oct 23, 2024

kratman requested a review from brosaplanella October 23, 2024 14:09

medha-14 force-pushed the gradient_square branch from 54edbdb to f8a506e Compare October 28, 2024 10:54

brosaplanella requested changes Oct 30, 2024

View reviewed changes

src/pybamm/spatial_methods/finite_volume.py Outdated Show resolved Hide resolved

src/pybamm/spatial_methods/finite_volume.py Outdated Show resolved Hide resolved

agriyakhetarpal reviewed Nov 7, 2024

View reviewed changes

agriyakhetarpal reviewed Nov 10, 2024

View reviewed changes

src/pybamm/spatial_methods/finite_volume.py Outdated Show resolved Hide resolved

src/pybamm/spatial_methods/finite_volume.py Show resolved Hide resolved

src/pybamm/spatial_methods/finite_volume.py Outdated Show resolved Hide resolved

agriyakhetarpal requested a review from brosaplanella November 10, 2024 23:04

agriyakhetarpal reviewed Feb 10, 2025

View reviewed changes

medha-14 requested a review from a team as a code owner February 19, 2025 10:15

medha-14 added 5 commits February 19, 2025 16:17

added gradient squared method

ae221ec

added test for gradient squared method

edb355e

modified gradient_squared to give a scalar value

c4681ac

"Modified gradient_squared to apply boundary conditions during proces…

5008452

…sing"

made suggested changes

aada57d

medha-14 force-pushed the gradient_square branch from 5a144b0 to aada57d Compare February 19, 2025 10:51

Merge branch 'develop' into gradient_square

d92b43e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `gradient_squared` method to `FiniteVolume` #4540

Adds `gradient_squared` method to `FiniteVolume` #4540

medha-14 commented Oct 23, 2024

codecov bot commented Oct 23, 2024 •

edited

Loading

kratman commented Oct 23, 2024

medha-14 commented Oct 28, 2024

brosaplanella left a comment

agriyakhetarpal Nov 7, 2024

medha-14 Nov 10, 2024

agriyakhetarpal left a comment

agriyakhetarpal commented Nov 10, 2024

medha-14 commented Nov 11, 2024

medha-14 commented Dec 11, 2024

agriyakhetarpal left a comment

agriyakhetarpal Feb 10, 2025

agriyakhetarpal Feb 10, 2025

agriyakhetarpal Feb 10, 2025

medha-14 commented Feb 10, 2025

		np.testing.assert_array_almost_equal(grad_squared_result, inner_product)
		np.testing.assert_array_less(0, grad_squared_result)

Adds gradient_squared method to FiniteVolume #4540

Are you sure you want to change the base?

Adds gradient_squared method to FiniteVolume #4540

Conversation

medha-14 commented Oct 23, 2024

Description

Type of change

Key checklist:

Further checks:

codecov bot commented Oct 23, 2024 • edited Loading

Codecov Report

kratman commented Oct 23, 2024

medha-14 commented Oct 28, 2024

brosaplanella left a comment

Choose a reason for hiding this comment

agriyakhetarpal Nov 7, 2024

Choose a reason for hiding this comment

medha-14 Nov 10, 2024

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal commented Nov 10, 2024

medha-14 commented Nov 11, 2024

medha-14 commented Dec 11, 2024

agriyakhetarpal left a comment

Choose a reason for hiding this comment

agriyakhetarpal Feb 10, 2025

Choose a reason for hiding this comment

agriyakhetarpal Feb 10, 2025

Choose a reason for hiding this comment

agriyakhetarpal Feb 10, 2025

Choose a reason for hiding this comment

medha-14 commented Feb 10, 2025

Adds `gradient_squared` method to `FiniteVolume` #4540

Adds `gradient_squared` method to `FiniteVolume` #4540

codecov bot commented Oct 23, 2024 •

edited

Loading