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.

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

#2188 implement evaluate at in 1D #3573

Merged
merged 9 commits into from
Dec 5, 2023
Merged

#2188 implement evaluate at in 1D #3573

merged 9 commits into from
Dec 5, 2023

Conversation

rtimms
Copy link
Contributor

@rtimms rtimms commented Nov 28, 2023

Description

Implement a new unary operataor EvaluateAt that evaluates a spatial variable at a given position. For now, this is only implemented for 1D spatial variables using the Finite Volume method. In this method, EvalauteAt returns the variable evaluated at the node closest to the requested value. E.g. EvaluateAt(3*x, 1.9) on a mesh with nodesx=[0.5, 2.5, 4.5] would return 7.5.

The motivation for this is to be able to place a reference electrode at a given point in the cell, to mimic a 3E setup.

Fixes #2188

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

Copy link

codecov bot commented Nov 28, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ebacf49) 99.59% compared to head (b2848f5) 99.58%.
Report is 15 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3573      +/-   ##
===========================================
- Coverage    99.59%   99.58%   -0.01%     
===========================================
  Files          257      257              
  Lines        20639    20707      +68     
===========================================
+ Hits         20556    20622      +66     
- Misses          83       85       +2     

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

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, suggesting a few changes to improve this.

Is the reason not to make this a standard variable that it doesn't work in >1D ? If so, how much work is it really to make it work in higher dimensions? Should just be some kind of kron of the matrix

examples/scripts/3E_cell.py Outdated Show resolved Hide resolved
pybamm/expression_tree/unary_operators.py Outdated Show resolved Hide resolved
index = np.argmin(np.abs(nodes - value))

# Create a sparse matrix with a 1 at the index
matrix = csr_matrix(([1], ([0], [index])), shape=(1, mesh.npts))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be too hard to update this to linearly interpolate between the two closest points

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure linearly interpolating is the right thing to do. Choosing the nearest value is consistent with FVM where the nodes are cell-centered and the function is piecewise constant. Also, you can construct examples where "closest two" isn't well defined. E.g. if the mesh nodes are [025, 0.5, 0.75] and I evaluate at 0.5, what should I do? This kind of example comes up naturally when you evaluate at the midpoint and you have an odd number of nodes.

Choosing the nearest node is basically like asking "which volume am I in?".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, makes sense

@rtimms
Copy link
Contributor Author

rtimms commented Nov 28, 2023

Is the reason not to make this a standard variable that it doesn't work in >1D ? If so, how much work is it really to make it work in higher dimensions? Should just be some kind of kron of the matrix

Was trying to avoid the complexity of passing in x,y,z,r etc. for now, but can spend more time on this to make it more general. It's probably straightforward for battery models where we can map the input variable to the dimension, but it's harder in general I think.

@valentinsulzer
Copy link
Member

Was trying to avoid the complexity of passing in x,y,z,r etc. for now, but can spend more time on this to make it more general. It's probably straightforward for battery models where we can map the input variable to the dimension, but it's harder in general I think.

Ah yeah I see. I think it's still ok if you limit it to primary domain and the domain has to be 1D

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Why can't the reference electrode always be inserted into the model, even for dimensions > 0 ?

@rtimms
Copy link
Contributor Author

rtimms commented Nov 30, 2023

Why can't the reference electrode always be inserted into the model, even for dimensions > 0 ?

Since EvaluateAt is only implemented for 1D and for the primary dimension only, when dimensionality>0 the placement of the reference electrode is no longer a single point, it's phi_e(position, y, z), so the definition of the 3E potentials is ambiguous.

Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@valentinsulzer
Copy link
Member

In a real system how does it work? Could we evaluate at the "tab" like we do for the voltage?

@brosaplanella
Copy link
Member

In a real system how does it work? Could we evaluate at the "tab" like we do for the voltage?

For the system I am (vaguely) familiar with, the EL cells, there is a lithium ring placed between an electrode and the separator. I think in other setups they just place like a small tab inside, but not sure how much precision they have when placing it.

@rtimms
Copy link
Contributor Author

rtimms commented Nov 30, 2023

Could we evaluate at the "tab" like we do for the voltage?

We can for the solid potentials, but where in y,z do we evaluate the electrolyte potential?

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, let's leave as separate method for now, please update changelog to document the separate method

@rtimms rtimms merged commit 9c85dd7 into develop Dec 5, 2023
33 of 35 checks passed
@rtimms rtimms deleted the issue-2188-evaluate-at branch December 5, 2023 17:07
@dion-w
Copy link
Contributor

dion-w commented Dec 7, 2023

Happy to see it implemented! Maybe in the future I can do it without my workaround of assessing different electrode potentials. Great work!

@rtimms rtimms restored the issue-2188-evaluate-at branch December 13, 2023 16:39
@kratman kratman deleted the issue-2188-evaluate-at branch March 19, 2024 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduction of 3 electrode setup
4 participants