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

Add single-layer SEI #2333

Open
valentinsulzer opened this issue Sep 30, 2022 · 12 comments · May be fixed by #3920
Open

Add single-layer SEI #2333

valentinsulzer opened this issue Sep 30, 2022 · 12 comments · May be fixed by #3920
Assignees
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows

Comments

@valentinsulzer
Copy link
Member

Currently the default for SEI is to have an inner and outer layer. This is only for historical reasons because Scott Marquis was investigating this in his [thesis].

In practice most people set both layers to be equal or set the inner layer to be zero. It would be good to have the option to only have a single layer SEI. This should probably be the default.

@valentinsulzer valentinsulzer added the difficulty: easy A good issue for someone new. Can be done in a few hours label Oct 11, 2022
@iatzak
Copy link
Contributor

iatzak commented Dec 4, 2022

I would like to work on this. Can you please assign this issue to me?

@valentinsulzer
Copy link
Member Author

Thanks @iatzak ! Let's work through required changes here before you write any code. If you have ideas on what to do please write out your plan here. If you're unsure or have any questions feel free to ask

@iatzak
Copy link
Contributor

iatzak commented Dec 5, 2022

Great, thanks for the availability to answer questions! I'm attending a course where the students are supposed to contribute to an open-source simulation software, and I have chosen PyBaMM. Until the Christmas break I'll be working on a report about PyBAMM's "infrastructure" (CI / documentation / building / contribution / git workflow). After the break, I'll get to this issue, firstly by writing here what I plan to do, as you suggested.

@valentinsulzer
Copy link
Member Author

That's awesome! If you can share the report or make it public, let us know what can be better, and contribute to making it better, that would be really helpful

@iatzak
Copy link
Contributor

iatzak commented Dec 5, 2022

Sure! :) I'll contribute with what I can. I should talk to the professor about sharing the report, but I see no reason to be prohibited.

@TomTranter
Copy link
Contributor

Hi @iatzak, how's it going? We are running a hackathon on 29th of March in Oxford and this issue is potentially one we could work on together. You'd be welcome to join us if you can make it. Tickets are free but limited availability https://www.eventbrite.co.uk/e/pybamm-hackathon-tickets-517334430207?keep_tld=1
If you have already made some progress on this issue then let us know and we will exclude it from the options to work on during the day. Thanks

@iatzak
Copy link
Contributor

iatzak commented Feb 4, 2023

Hi @TomTranter, thanks for the heads up. I'm working on this later than I expected due to university courses, however I still plan to contribute to this issue.

@tinosulzer could you describe shortly in which files the changes need to be made?

@jeromtom
Copy link
Contributor

jeromtom commented Feb 4, 2023

@iatzak
The changes are to be made in this folder pybamm\models\submodels\interface\sei
You will have to create a new python file something like single_layer_sei.py with the parameter L_inner set to zero and keep that as default, allowing the user to change L_outer to zero if needed. @tinosulzer, please confirm.

Documentation for current SEI models

@valentinsulzer
Copy link
Member Author

No need to create a new python file. Just use the same file and read in the option "number of SEI layers" (which you'll have to add to BatteryModelOptions). If it's "2", leave as is. If it's "1", only create a single variable L_sei (instead of L_inner and L_outer), and then only write equations for that variable. There will be a few places in the code where we assumed that there are two SEI thicknesses, and you'll need to edit those too, again by reading self.options["number of SEI layers"]

@jeromtom
Copy link
Contributor

Hey @iatzak, I really would like to work on this issue, let me know if you are still busy. Thanks.

@iatzak
Copy link
Contributor

iatzak commented Feb 24, 2023

Hi @jeromtom, you can go ahead, since I made my course-related contribution on another issue and I would only start working on this one in the next week.

@iatzak iatzak removed their assignment Feb 24, 2023
@rtimms rtimms added the priority: medium To be resolved if time allows label May 15, 2023
@GBlanka
Copy link

GBlanka commented Oct 13, 2023

Hi @tinosulzer, I would like to work on this issue, could you please assign it to me?

@GBlanka GBlanka mentioned this issue Oct 18, 2023
8 tasks
GBlanka added a commit to GBlanka/PyBaMM that referenced this issue Oct 22, 2023
GBlanka added a commit to GBlanka/PyBaMM that referenced this issue Oct 22, 2023
@lorenzofavaro lorenzofavaro linked a pull request Mar 22, 2024 that will close this issue
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy A good issue for someone new. Can be done in a few hours priority: medium To be resolved if time allows
Projects
None yet
8 participants