-
Notifications
You must be signed in to change notification settings - Fork 11
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
Update conversion electrode and non-lithiated sulfur input file #79
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @korffdm -- excited to get these capabilities folded in!
I have left a bunch of comments, mostly related to generalizing this a little bit more (i.e. removing references to cathode, sulfur, etc). I haven't fully finished my run through, but thought it worth posting this first round. Also, take a look at the version already on GitHub--I think there are a few tidbits you could use, particularly related to capacity calculation.
Thanks--looking really good!
THe model assumes some intert, electrically conductive host phase (e.g. carbon), plus an arbitrary number of "conversion" phases. Each conversion phase can possibly participate in reactions at the host/electrolyte interface, plus at their own electrolyte interface. | ||
|
||
Three phase boundaries are not currently implemented, but certainly could be, at some later date. | ||
Class file for air electrode methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think my previous change should supersede this. I'm not sure where these diverged in commit history, but I am guessing, sadly, that a simple rebase would not fix it. But it is worth a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the comment that used to be here:
Class file for conversion electrode methods. An example is lithium-sulfur, where solid S8 phase is converted to Li2S during discharge.
The model assumes some intert, electrically conductive host phase (e.g. carbon), plus an arbitrary number of "conversion" phases. Each conversion phase can possibly participate in reactions at the host/electrolyte interface, plus at their own electrolyte interface.
Three phase boundaries are not currently implemented, but certainly could be, at some later date.
Something like this would be good, but I think that last sentence is no longer accurate--is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, three phase boundaries are included now.
""" | ||
|
||
def __init__(self, input_file, inputs, sep_inputs, counter_inputs, | ||
def __init__(self, input_file, inputs, sep_inputs, an_inputs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if/how an_inputs
gets used, but again - we need to generalize this. I previously had counter_inputs
, which is a little better ("counter" = "other," I guess), but still not great. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't currently get used in conversion_electrode.py
, but it could depending on the capacity calculation (capacity due to material/species in electrolyte). I was going to ask about that in our next meeting. I see the issue of calling it explicitly an_inputs
though. I think if we change it back to using the 'counter' terminology, possibly add counter_ed_inputs
so that it's a little more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me - thanks!
self.n_conversion_phases += 1 | ||
|
||
# Anode or cathode? Positive external current delivers positive charge | ||
# to the anode, and removes positive charge from the cathode. | ||
E0_ca = self.host_surf_obj.delta_standard_gibbs/ct.faraday |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove ca
reference here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this line go away, entirely? Can no longer find it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It did, I removed print statements that I'd used for debugging.
+ 8*self.conversion_obj[0].density_mole*self.eps_conversion_init[0]*inputs['thickness'] | ||
+ self.conversion_obj[1].density_mole*self.eps_conversion_init[1]*inputs['thickness']) | ||
|
||
W_S = (self.conversion_obj[0].molecular_weights/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of stuff specific to Sulfur, here. Can we generalize it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now!
|
||
m_S_el = eps_el_0*inputs['thickness']*W_S*np.dot(n_S_atoms, self.C_k_0) | ||
|
||
self.capacity = 1675*0.01952351 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version of the code in main
has a more generalized capacity calculation that I think you could copy, to replace this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I got the generalization figured out. I do have an assumption right now that there's only two conversion phases. I think to do more we would need one or two more input fields in the yaml, like an end_product_species: Li2S(s)
maybe?
Also removed all printing statements for debugging.
No description provided.