Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Bug]: Coordinate systems of variables do not get checked against known #3114

Open
brosaplanella opened this issue Jul 6, 2023 · 4 comments · May be fixed by #3394
Open

[Bug]: Coordinate systems of variables do not get checked against known #3114

brosaplanella opened this issue Jul 6, 2023 · 4 comments · May be fixed by #3394
Assignees
@RuiheLi
Labels
bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows

Comments

@brosaplanella
Copy link
Sponsor Member

brosaplanella commented Jul 6, 2023 •
edited

PyBaMM Version

23.5

Python Version

3.9.16

Describe the bug

Currently the coordinate system of a variable is not checked against the known system. This means that if the wrong argument is passed (e.g. due to a typo) it will default to "cartesian" without throwing an error.

Steps to Reproduce

The MWE would be very lengthy, but basically it can be checked that:

  1. The only place where KNOWN_COORD_SYS is checked is to test meshes for the battery model (i.e. it does not actually check for wrong coordinate systems).
  2. In finite_volume.py there is an else statement that means that anything not being cylindrical polar or spherical polar will default to cartesian, see

PyBaMM/pybamm/spatial_methods/finite_volume.py

Lines 139 to 150 in fffe650

# check coordinate system
if submesh.coord_sys in ["cylindrical polar", "spherical polar"]:
second_dim_repeats = self._get_auxiliary_domain_repeats(symbol.domains)
# create np.array of repeated submesh.edges
r_edges_numpy = np.kron(np.ones(second_dim_repeats), submesh.edges)
r_edges = pybamm.Vector(r_edges_numpy)
if submesh.coord_sys == "spherical polar":
out = divergence_matrix @ ((r_edges**2) * discretised_symbol)
elif submesh.coord_sys == "cylindrical polar":
out = divergence_matrix @ (r_edges * discretised_symbol)
else:
out = divergence_matrix @ discretised_symbol

Relevant log output

No response
@brosaplanella brosaplanella added bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows labels Jul 6, 2023
@brosaplanella
Copy link
Sponsor Member Author

Ooops, that was already reported in #2220 (contains some additional info). Let's make sure we close both issues when we merge PR.

@brosaplanella
Copy link
Sponsor Member Author

@RuiheLi are you working on this?

@RuiheLi
Copy link
Contributor

RuiheLi commented Jul 25, 2023

Yes, I will. Will look into #2220. Thanks for the remind! @brosaplanella

@RuiheLi
Copy link
Contributor

RuiheLi commented Jul 25, 2023

Yes, I will. Will look into #2220. Thanks for the remind! @brosaplanella

Just checked the time of both issue, will look into both.

@kratman kratman linked a pull request Jan 26, 2024 that will close this issue
Open
8 tasks
@kratman kratman mentioned this issue May 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@RuiheLi RuiheLi

Labels
bug Something isn't working difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows
Projects
August 2023 sprint
Status: In Progress
September 2023 sprint
Status: In Progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue 3114 check coord RuiheLi/PyBaMM
4 participants
@brosaplanella @Saransh-cpp @agriyakhetarpal @RuiheLi