Issue 3114 check coord

[RuiheLi]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #3114

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

[RuiheLi]

sorry I accidentally create another one

[agriyakhetarpal]

I think this was the correct way, because it is from a feature branch to the develop branch. @RuiheLi, you may reopen this pull request and cherry-pick your commits from the develop branch (#3378, which can be closed afterwards) to this branch so that you don't lose progress

[RuiheLi]

I think this was the correct way, because it is from a feature branch to the develop branch. @RuiheLi, you may reopen this pull request and cherry-pick your commits from the develop branch (#3378, which can be closed afterwards) to this branch so that you don't lose progress

Thank you so much for the kind suggestion. Reopen now.
All the commits before should be alright because I cereated the branch issue_3114_check_coord based on the develop branch.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5322895) 99.58% compared to head (15402f2) 99.57%.
Report is 411 commits behind head on develop.

❗ Current head 15402f2 differs from pull request most recent head 6df9a16. Consider uploading reports for the commit 6df9a16 to get more accurate results

Files	Patch %	Lines
pybamm/spatial_methods/finite_volume.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3394      +/-   ##
===========================================
- Coverage    99.58%   99.57%   -0.01%     
===========================================
  Files          256      256              
  Lines        20131    20137       +6     
===========================================
+ Hits         20048    20052       +4     
- Misses          83       85       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[brosaplanella]

Looks good, thanks! Before I can approve, can you add a line to the CHANGELOG?

You can copy the format of one of the other lines, but you will need to add it under the "Unreleased" section rather than 23.9.

[brosaplanella]

@all-contributors please add @RuiheLi for code and tests

[allcontributors]

@brosaplanella

This project's configuration file has malformed JSON: .all-contributorsrc. Error:: Unexpected token ] in JSON at position 18423

[brosaplanella]

@all-contributors please add @RuiheLi for code and tests

[allcontributors]

@brosaplanella

I've put up a pull request to add @RuiheLi! 🎉

[kratman]

One more minor change. It is generally best to end an if-else block with an else. That way future developers know the branches were meant to end there and that something was not just forgotten.

Suggested change

	elif coord_sys is not None:
	raise ValueError(
	f"Coordinate system is {coord_sys}, not in"
	" cartesian, cylindrical polar, spherical polar, None")
	else:
	if coord_sys is not None:
	raise ValueError(
	f"Coordinate system is {coord_sys}, not in"
	" cartesian, cylindrical polar, spherical polar, None"
	)

[brosaplanella]

Also note that this line is not covered so an additional test is needed.

[RuiheLi]

ok, that comes something I am very weak now. Just reading the test_finite_volume.py file and only come up with:

def test_divergence(self):
        whole_cell = ["negative electrode", "separator", "positive electrode"]
        var = pybamm.Variable("var", domain=whole_cell)
        x = pybamm.SpatialVariableEdge(
                "x_s",
                domain=["separator"],
                auxiliary_domains={"secondary": "current collector"},
                coord_sys="cartesian_2",
            )
        with self.assertRaises(ValueError):
            pybamm.SpatialMethod.divergence(var,x)

[RuiheLi]

but I am pretty sure this won't work, so just to raise a question.

[brosaplanella]

This test is similar to the one you have, which is necessary because it checks the error is raised in independent_variable.py. You need another test that calls directly the divergence method in finite_volume.py. However, I am not sure we can do that test without triggering the other error before. That's because in order to define the symbol to pass to divergence we need to give it a domain with its own coordinate system. @kratman, any thoughts?

[RuiheLi]

Thank you for the kind reply. @brosaplanella That's something I am a bit struggled as well. Though I notice that I can get some mesh by mesh = get_mesh_for_testing(). If I look closer at get_mesh_for_testing, they already assign coordinate system, and I don't know how to change that.

[kratman]

Please update the PR description so that 3114 gets closed by this change (use the fixes tag in the template)

[kratman]

@RuiheLi Are you still working on this?

[RuiheLi]

@RuiheLi Are you still working on this?

I have added the final else as suggested. But I had a try and still cannot write a proper test for the divergence as it will require to define a spatial variable with incorrect coordinate system first which will trigger an early error.

[brosaplanella]

I have added the final else as suggested. But I had a try and still cannot write a proper test for the divergence as it will require to define a spatial variable with incorrect coordinate system first which will trigger an early error.

@kratman what's the best way to approach that? Should we add a # pragma: no cover tag?

[kratman]

I will take a look at this closer in the afternoon and give my opinion on the tests

[rtimms]

there are a few other places where there is similar logic that should be updated (do a search for submesh.coord_sys in ["cylindrical polar", "spherical polar"])

[rtimms]

I have added the final else as suggested. But I had a try and still cannot write a proper test for the divergence as it will require to define a spatial variable with incorrect coordinate system first which will trigger an early error.

@kratman what's the best way to approach that? Should we add a # pragma: no cover tag?

now that the coordinate system is already checked should the logic in discretisation be changed? i.e. we don't need to throw that error and the final else can just be the Cartesian case? alternatively, if we want to keep the error and have a test you could create a spatial variable with the correct domain, then manually overwrite cood_sys for the submesh later and then create the divergence object. seems clunky though

[kratman]

@RuiheLi Sorry it took so long for me to respond, but I think there is a nice workaround to test this change without ignoring any lines.

If you are having trouble testing that function, then it might get easier if you create a function for the block in question. If you create a function called something along the lines of "check_coordinate_system()" for the if-elif-else block, then it can take all things it is supposed to check as arguments. That might make it easier to write a test without adding making the test too complicated. I did something similar in this commit: fa6bdc2

In general short functions are easy to test. So breaking up complicated functions is probably a good thing anyway. Whenever it is difficult to test a function and we consider adding a pragma to ignore it. It is often a sign that we need to break up the function into smaller testable methods.

[rtimms]

thanks @kratman, i like this approach as we check coordinates a few times in that file, so will reduce duplication too

[kratman]

@RuiheLi Are you still working on this?