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

electrode tab coordinate inputs #964

Closed
gyouhoc opened this issue Apr 17, 2020 · 10 comments · Fixed by #993
Closed

electrode tab coordinate inputs #964

gyouhoc opened this issue Apr 17, 2020 · 10 comments · Fixed by #993
Assignees

Comments

@gyouhoc
Copy link
Contributor

gyouhoc commented Apr 17, 2020

Summary
I think some of the example parameters such as LG cell provided with the software some electrode tab location numbers are not correct.

For instance,

Electrode height [m] = 0.065
Negative tab centre y-coordinate [m] = 0.06 <= does this mean the tab is located in the electrode coating? ( I might be confused).
Would you direct me to the page where I have more information or easy pic to understand the coordinate system?

Capture

Also, for the jellyroll structure, do I count for the entire length of the jellyroll?

@rtimms
Copy link
Contributor

rtimms commented Apr 18, 2020

It looks like these values are incorrect and haven’t been updated from the Marquis parameter set. I think if you tried to use these parameters with the option “dimensionality” equal to 2 it should throw an error (the coordinates for the tab centre must be located at the edge of the cell). @ferranbrosa would you be able to update this with the tab location for this cell?

Yeah for the jelly roll these dimensions are as if you “unrolled” the cell, so the height x width gives the electrode surface area. Note that at the moment the thermal models assume a pouch cell geometry, see #718

@brosaplanella
Copy link
Member

Yes, these values have not been updated as the structure is not that of a pouch cell and the tabs are significantly different (for example, they are asymmetric depending on the side of the current collector). I think some modifications in the model for cylindrical batteries would be needed in order to account for the LGM50 geometry.

In terms of the electrode plate area, we defined it as the total area (accounting for the double coating of the electrodes) so the applied current density is I/A.

@rtimms
Copy link
Contributor

rtimms commented Apr 20, 2020

Ah ok. Maybe we would be best to remove these or set them to "-" or similar to avoid confusion? Provided that doesn't break everything

@brosaplanella
Copy link
Member

Yes, I left the default number just in case things broke down. Is there an error to throw in these cases (like non-available parameter or so)? Not sure if this relates to #963

@rtimms
Copy link
Contributor

rtimms commented Apr 20, 2020

Ah yeah I think it will break if you don't supply it. I guess we should allow for a parameter to be missing, provided it isn't used in the model, and only raise the error when the model tries to use a parameter that is missing.

@valentinsulzer
Copy link
Member

only raise the error when the model tries to use a parameter that is missing.

This should be what happens already, if there's an error that means the parameter is being used somewhere in the nondimensionalisation

@brosaplanella
Copy link
Member

Cool! Have we agreed a convention for missing parameters?

@valentinsulzer
Copy link
Member

Don't think so, I would say just leave it out of the parameter set?

@brosaplanella
Copy link
Member

Ok! In this case, given that some parameter sets will have some extra fields, shall we define a standard format (with all the fields appearing in the same order and removed if not needed) so it is easier for a human to read? Or is it not worth it?

@rtimms
Copy link
Contributor

rtimms commented Apr 22, 2020

related: could we add a method that prints all the parameters a model requires?

Edit: I suspect for now most of the models will need most of the parameters we currently supply, but such a method may be useful as we add more physics

@rtimms rtimms self-assigned this May 13, 2020
rtimms added a commit that referenced this issue May 13, 2020
@rtimms rtimms mentioned this issue May 13, 2020
8 tasks
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 a pull request may close this issue.

4 participants