Fixes #3441 call processed variables with custom spatial coordinates

[Saswatsusmoy]

Description

Modified the call method to accept **kwargs as a parameter. This will allow any additional keyword arguments to be passed to the method. You can then include these extra arguments when calling the interp method of the _xr_data_array object.

Fixes #3441

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

[kratman]

Can you add some tests for this?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (84bfd61) 99.59% compared to head (bbc63df) 99.59%.

❗ Current head bbc63df differs from pull request most recent head 22caa58. Consider uploading reports for the commit 22caa58 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3472      +/-   ##
===========================================
- Coverage    99.59%   99.59%   -0.01%     
===========================================
  Files          257      257              
  Lines        20806    20804       -2     
===========================================
- Hits         20722    20720       -2     
  Misses          84       84              

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

[Saswatsusmoy]

@rtimms I have tried solving the issue, can you please review the same

[brosaplanella]

Could you improve the tests so they actually tests that the variables are passed correctly? At the moment it only checks that no error message is thrown.

[Saswatsusmoy]

def test_call_default(self, mock_call_func):
        call_func()
        mock_call_func.assert_called()

    @patch('path.to.call_func')
    def test_call_with_t(self, mock_call_func):
        t_test = 0.5
        call_func(t=t_test)
        mock_call_func.assert_called_with(t=t_test)
.
.
.
.

Something like this? @brosaplanella

[brosaplanella]

I was thinking something more PyBaMM specific. Maybe something like this example (

PyBaMM/tests/unit/test_solvers/test_processed_variable.py

Lines 139 to 163 in 369034f

	t = pybamm.t
	var = pybamm.Variable("var", domain=["negative electrode", "separator"])
	x = pybamm.SpatialVariable("x", domain=["negative electrode", "separator"])
	eqn = t * var + x
	# On nodes
	disc = tests.get_discretisation_for_testing()
	disc.set_variable_slices([var])
	x_sol = disc.process_symbol(x).entries[:, 0]
	var_sol = disc.process_symbol(var)
	eqn_sol = disc.process_symbol(eqn)
	t_sol = np.linspace(0, 1)
	y_sol = np.ones_like(x_sol)[:, np.newaxis] * np.linspace(0, 5)
	var_casadi = to_casadi(var_sol, y_sol)
	processed_var = pybamm.ProcessedVariable(
	[var_sol],
	[var_casadi],
	pybamm.Solution(
	t_sol, y_sol, tests.get_base_model_with_battery_geometry(), {}
	),
	warn=False,
	)
	np.testing.assert_array_equal(processed_var.entries, y_sol)
	np.testing.assert_array_almost_equal(processed_var(t_sol, x_sol), y_sol)

), where a dummy model is created but instead of calling your space variable x, try calling it something that would be a kwarg above to see if it is passed correctly. Maybe call it a and try both passing a to see it works and passing b to check it throws an error.

[Saswatsusmoy]

def call_func(t=None, a=None, b=None): if a is not None: return a + 1 elif b is not None: return b + 2 else: return np.array([0])

`class TestProcessedVariableComputedCalls(unittest.TestCase):
def test_call_kwargs(self):
var = pybamm.Variable("var")
a = pybamm.InputParameter("a")
model = pybamm.BaseModel()
geom = pybamm.Geometry()
mesh = pybamm.Mesh(geom)
var.mesh = mesh

    t = np.linspace(0,1)
    y = np.zeros(1)

    inputs = {"a": 1}

    fun = to_casadi(var, y, inputs)

    solution = pybamm.Solution(t, y, model, inputs)
    processed_var = pybamm.ProcessedVariableComputed([var], [fun], [y], solution)

    # Test passing correct kwarg
    result = processed_var(a=1)
    self.assertEqual(result, 2)

    # Test passing incorrect kwarg
    with self.assertRaises(TypeError):
        processed_var(b=1)`

Something like this? @brosaplanella

The key changes are:

Define a simple call_func that takes kwargs
Create a dummy model/variable/solution
Convert the variable to casadi function
Create the ProcessedVariableComputed
Test calling it with correct and incorrect kwargs

[brosaplanella]

I think so. Note that you want a to be a SpatialVariable rather than an InputParameter because the latter needs to be passed at solving stage.

[kratman]

@Saswatsusmoy Are you still working on this?

[Saswatsusmoy]

@Saswatsusmoy Are you still working on this?

Yeah, I'll be resuming working on this soon

[arjxn-py]

@Saswatsusmoy Feel free to let us know in case you feel stuck with something, Maybe we'd be able to help you out.

[agriyakhetarpal]

We have not had a response from the contributor in four months by now. I think we should be fine with closing the PR, we can reopen whenever – especially because the linked issue is labelled as "low priority" at the moment