Repository: pybamm-team/PyBaMM (python)

Issue:
sign error in fem dirichlet bcs
In FEM dirichlet BCs have the wrong sign 

Pull request:
#705 fix sign error
# Description
Fixes a sign error when implementing Dirichlet BCs in the finite element method

EDIT: also fixes a bug where spatial variables were discretised to a vector of length N_y or N_z for y and z, respectively, instead of a vector of length N_y*N_z

Fixes #705 

## Type of change

Please add a line in the relevant section of [CHANGELOG.md](https://github.com/pybamm-team/PyBaMM/blob/master/CHANGELOG.md) to document the change (include PR #) - note reverse order of PR #s.

- [ ] New feature (non-breaking change which adds functionality)
- [ ] Optimization (back-end change that speeds up the code)
- [x] Bug fix (non-breaking change which fixes an issue)


# Key checklist:

- [x] No style issues: `$ flake8`
- [x] All tests pass: `$ python run-tests.py --unit`
- [x] The documentation builds: `$ cd docs` and then `$ make clean; make html`

You can run all three at once, using `$ python run-tests.py --quick`.

## Further checks:

- [ ] Code is commented, particularly in hard-to-understand areas
- [x] Tests added that prove fix is effective or that feature works


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index dde65eed1..ffd77754d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,7 @@
 
 ## Bug fixes
 
+-   Correct a sign error in Dirichlet boundary conditions in the Finite Element Method ([#706](https://github.com/pybamm-team/PyBaMM/pull/706))
 -   Pass the correct dimensional temperature to open circuit potential ([#702](https://github.com/pybamm-team/PyBaMM/pull/702))
 -   Adds missing temperature dependence in electrolyte and interface submodels ([#698](https://github.com/pybamm-team/PyBaMM/pull/698))
 -   Fix differentiation of functions that have more than one argument ([#687](https://github.com/pybamm-team/PyBaMM/pull/687))
diff --git a/pybamm/spatial_methods/scikit_finite_element.py b/pybamm/spatial_methods/scikit_finite_element.py
index ddf501049..464a1d68a 100644
--- a/pybamm/spatial_methods/scikit_finite_element.py
+++ b/pybamm/spatial_methods/scikit_finite_element.py
@@ -51,11 +51,11 @@ class ScikitFiniteElement(pybamm.SpatialMethod):
         symbol_mesh = self.mesh
         if symbol.name == "y":
             vector = pybamm.Vector(
-                symbol_mesh["current collector"][0].edges["y"], domain=symbol.domain
+                symbol_mesh["current collector"][0].coordinates[0, :][:, np.newaxis]
             )
         elif symbol.name == "z":
             vector = pybamm.Vector(
-                symbol_mesh["current collector"][0].edges["z"], domain=symbol.domain
+                symbol_mesh["current collector"][0].coordinates[1, :][:, np.newaxis]
             )
         else:
             raise pybamm.GeometryError(
@@ -121,7 +121,7 @@ class ScikitFiniteElement(pybamm.SpatialMethod):
             # set Dirichlet value at facets corresponding to tab
             neg_bc_load = np.zeros(mesh.npts)
             neg_bc_load[mesh.negative_tab_dofs] = 1
-            boundary_load = boundary_load - neg_bc_value * pybamm.Vector(neg_bc_load)
+            boundary_load = boundary_load + neg_bc_value * pybamm.Vector(neg_bc_load)
         else:
             raise ValueError(
                 "boundary condition must be Dirichlet or Neumann, not '{}'".format(
@@ -138,7 +138,7 @@ class ScikitFiniteElement(pybamm.SpatialMethod):
             # set Dirichlet value at facets corresponding to tab
             pos_bc_load = np.zeros(mesh.npts)
             pos_bc_load[mesh.positive_tab_dofs] = 1
-            boundary_load = boundary_load - pos_bc_value * pybamm.Vector(pos_bc_load)
+            boundary_load = boundary_load + pos_bc_value * pybamm.Vector(pos_bc_load)
         else:
             raise ValueError(
                 "boundary condition must be Dirichlet or Neumann, not '{}'".format(


Test patch:
diff --git a/tests/unit/test_processed_variable.py b/tests/unit/test_processed_variable.py
index a891fdbc6..87c29a84a 100644
--- a/tests/unit/test_processed_variable.py
+++ b/tests/unit/test_processed_variable.py
@@ -140,13 +140,11 @@ class TestProcessedVariable(unittest.TestCase):
 
     def test_processed_variable_3D_scikit(self):
         var = pybamm.Variable("var", domain=["current collector"])
-        y = pybamm.SpatialVariable("y", domain=["current collector"])
-        z = pybamm.SpatialVariable("z", domain=["current collector"])
 
         disc = tests.get_2p1d_discretisation_for_testing()
         disc.set_variable_slices([var])
-        y_sol = disc.process_symbol(y).entries[:, 0]
-        z_sol = disc.process_symbol(z).entries[:, 0]
+        y = disc.mesh["current collector"][0].edges["y"]
+        z = disc.mesh["current collector"][0].edges["z"]
         var_sol = disc.process_symbol(var)
         t_sol = np.linspace(0, 1)
         u_sol = np.ones(var_sol.shape[0])[:, np.newaxis] * np.linspace(0, 5)
@@ -154,25 +152,23 @@ class TestProcessedVariable(unittest.TestCase):
         processed_var = pybamm.ProcessedVariable(var_sol, t_sol, u_sol, mesh=disc.mesh)
         np.testing.assert_array_equal(
             processed_var.entries,
-            np.reshape(u_sol, [len(y_sol), len(z_sol), len(t_sol)]),
+            np.reshape(u_sol, [len(y), len(z), len(t_sol)]),
         )
 
     def test_processed_variable_2Dspace_scikit(self):
         var = pybamm.Variable("var", domain=["current collector"])
-        y = pybamm.SpatialVariable("y", domain=["current collector"])
-        z = pybamm.SpatialVariable("z", domain=["current collector"])
 
         disc = tests.get_2p1d_discretisation_for_testing()
         disc.set_variable_slices([var])
-        y_sol = disc.process_symbol(y).entries[:, 0]
-        z_sol = disc.process_symbol(z).entries[:, 0]
+        y = disc.mesh["current collector"][0].edges["y"]
+        z = disc.mesh["current collector"][0].edges["z"]
         var_sol = disc.process_symbol(var)
         t_sol = np.array([0])
         u_sol = np.ones(var_sol.shape[0])[:, np.newaxis]
 
         processed_var = pybamm.ProcessedVariable(var_sol, t_sol, u_sol, mesh=disc.mesh)
         np.testing.assert_array_equal(
-            processed_var.entries, np.reshape(u_sol, [len(y_sol), len(z_sol)])
+            processed_var.entries, np.reshape(u_sol, [len(y), len(z)])
         )
 
     def test_processed_var_1D_interpolation(self):
@@ -367,13 +363,11 @@ class TestProcessedVariable(unittest.TestCase):
 
     def test_processed_var_3D_scikit_interpolation(self):
         var = pybamm.Variable("var", domain=["current collector"])
-        y = pybamm.SpatialVariable("y", domain=["current collector"])
-        z = pybamm.SpatialVariable("z", domain=["current collector"])
 
         disc = tests.get_2p1d_discretisation_for_testing()
         disc.set_variable_slices([var])
-        y_sol = disc.process_symbol(y).entries[:, 0]
-        z_sol = disc.process_symbol(z).entries[:, 0]
+        y_sol = disc.mesh["current collector"][0].edges["y"]
+        z_sol = disc.mesh["current collector"][0].edges["z"]
         var_sol = disc.process_symbol(var)
         t_sol = np.linspace(0, 1)
         u_sol = np.ones(var_sol.shape[0])[:, np.newaxis] * np.linspace(0, 5)
@@ -406,13 +400,11 @@ class TestProcessedVariable(unittest.TestCase):
 
     def test_processed_var_2Dspace_scikit_interpolation(self):
         var = pybamm.Variable("var", domain=["current collector"])
-        y = pybamm.SpatialVariable("y", domain=["current collector"])
-        z = pybamm.SpatialVariable("z", domain=["current collector"])
 
         disc = tests.get_2p1d_discretisation_for_testing()
         disc.set_variable_slices([var])
-        y_sol = disc.process_symbol(y).entries[:, 0]
-        z_sol = disc.process_symbol(z).entries[:, 0]
+        y_sol = disc.mesh["current collector"][0].edges["y"]
+        z_sol = disc.mesh["current collector"][0].edges["z"]
         var_sol = disc.process_symbol(var)
         t_sol = np.array([0])
         u_sol = np.ones(var_sol.shape[0])[:, np.newaxis]
diff --git a/tests/unit/test_spatial_methods/test_scikit_finite_element.py b/tests/unit/test_spatial_methods/test_scikit_finite_element.py
index 5a108e275..21bd4b879 100644
--- a/tests/unit/test_spatial_methods/test_scikit_finite_element.py
+++ b/tests/unit/test_spatial_methods/test_scikit_finite_element.py
@@ -461,6 +461,68 @@ class TestScikitFiniteElement(unittest.TestCase):
         u_exact = z ** 2 / 2 - 1 / 6
         np.testing.assert_array_almost_equal(solution.y[:-1], u_exact, decimal=1)
 
+    def test_dirichlet_bcs(self):
+        # manufactured solution u = a*z^2 + b*z + c
+        model = pybamm.BaseModel()
+        a = 3
+        b = 4
+        c = 5
+        u = pybamm.Variable("variable", domain="current collector")
+        model.algebraic = {u: -pybamm.laplacian(u) + pybamm.source(2 * a, u)}
+        # set boundary conditions ("negative tab" = bottom of unit square,
+        # "positive tab" = top of unit square, elsewhere normal derivative is zero)
+        model.boundary_conditions = {
+            u: {
+                "negative tab": (pybamm.Scalar(c), "Dirichlet"),
+                "positive tab": (pybamm.Scalar(a + b + c), "Dirichlet"),
+            }
+        }
+        # bad initial guess (on purpose)
+        model.initial_conditions = {u: pybamm.Scalar(1)}
+        model.variables = {"u": u}
+        # create discretisation
+        mesh = get_unit_2p1D_mesh_for_testing(ypts=8, zpts=32)
+        spatial_methods = {
+            "macroscale": pybamm.FiniteVolume,
+            "current collector": pybamm.ScikitFiniteElement,
+        }
+        disc = pybamm.Discretisation(mesh, spatial_methods)
+        disc.process_model(model)
+
+        # solve model
+        solver = pybamm.AlgebraicSolver()
+        solution = solver.solve(model)
+
+        # indepedent of y, so just check values for one y
+        z = mesh["current collector"][0].edges["z"][:, np.newaxis]
+        u_exact = a * z ** 2 + b * z + c
+        np.testing.assert_array_almost_equal(solution.y[0 : len(z)], u_exact)
+
+    def test_disc_spatial_var(self):
+        mesh = get_unit_2p1D_mesh_for_testing(ypts=4, zpts=5)
+        spatial_methods = {
+            "macroscale": pybamm.FiniteVolume,
+            "current collector": pybamm.ScikitFiniteElement,
+        }
+        disc = pybamm.Discretisation(mesh, spatial_methods)
+
+        # discretise y and z
+        y = pybamm.SpatialVariable("y", ["current collector"])
+        z = pybamm.SpatialVariable("z", ["current collector"])
+        y_disc = disc.process_symbol(y)
+        z_disc = disc.process_symbol(z)
+
+        # create expected meshgrid
+        y_vec = np.linspace(0, 1, 4)
+        z_vec = np.linspace(0, 1, 5)
+        Y, Z = np.meshgrid(y_vec, z_vec)
+        y_actual = np.transpose(Y).flatten()[:, np.newaxis]
+        z_actual = np.transpose(Z).flatten()[:, np.newaxis]
+
+        # spatial vars should discretise to the flattend meshgrid
+        np.testing.assert_array_equal(y_disc.evaluate(), y_actual)
+        np.testing.assert_array_equal(z_disc.evaluate(), z_actual)
+
 
 if __name__ == "__main__":
     print("Add -v for more debug output")