Open morrowrasmus opened 4 months ago
@morrowrasmus, I agree on all points :-)
Make it reasonably simple for the first implementation, and then we can build upon that later when or if we have a robust fitting methodology. One design question we would need to make at one point is if we should add more than one set of IR columns to the summary
table. Since "IR" can be obtained both from specific steps implemented by the testers themselves, and from post-processing steps (as suggested here)...
I don't think it makes sense anymore to include this in the steps
-table at all, and we should instead just add the four values mentioned above to the summary
-table.
As I understood from our previous conversation, the _ir_to_summary()
-method is in meant to pick out already calculated values from the cyclers, right? In that case we should not touch this, and instead include a new method, e.g. _calculated_ir_to_summary()
which can be invoked with an addition to summary_kwargs
called calculate_ir
(as opposed to find_ir
which exists already) that is completely independent of existing functionality. This could be called right after _ir_to_summary
. This method would need to go through the steps table and pick out the first and last values of the potential and current of the correct steps in the correct order*. I think this is as good a place to put this as any, as the data we need should be readily available in the already calculated steps
-table.
One design question we would need to make at one point is if we should add more than one set of IR columns to the
summary
table. Since "IR" can be obtained both from specific steps implemented by the testers themselves, and from post-processing steps (as suggested here)...
I agree, we should in any case keep the original behaviour in tact so it doesn't break anything for anyone. Now the headers are defined in HeadersSummary
is "ir_charge"
and "ir_discharge"
. Could we call the new calculated values "ir_charge_start_calculated"
, "ir_charge_end_calculated"
, "ir_discharge_start_calculated"
, "ir_discharge_end_calculated"
and keep six columns if both find_ir
and calculate_ir
are set to True
?
I can see that there is some functionality to classify a step where there is no change as ir
:
# --- internal resistance ----
df_steps.loc[mask_no_change, (shdr.type, slice(None))] = "ir"
# assumes that IR is stored in just one row
I am guessing this also is based on picking out IR-values from Maccor-cyclers?
* For the moment we have turned the sorting of rows off (step_kwargs={'sort_rows': False}
) as we now don't include test_time
in the data file we import at all. Have you experienced much issues with steps out of order?
Is your feature request related to a problem? Please describe. Based on the potential drop/jump at the end/start of (dis)charge, the internal resistance should be calculated and reported in the
steps
-DataFrame and possibly in thesummary
-DataFrame.Describe the solution you'd like A simple estimation is suggested: take the potential change between the last point in a preceeding rest step and the first point in a charge or discharge step, and calculate the estimated internal resistance based on Ohm's law. The same for last point of the end of the (dis)charge step and first point of the subsequent rest step. This should result in two columns in the
steps
-table (ir_start
,ir_end
) and four columns in thesummary
-DataFrame if impemented there (ir_start_charge
,ir_start_discharge
,ir_end_charge
,ir_end_discharge
)As a part of this, the relaxation potential and OCV-potential estimation can also be added to the
steps
-DataFrame(and possibly thesummary
-DataFrame) based on the first and last potential values of the following rest-step. This will also result in two columns in thesteps
-DataFrame and four columns in thesummary
-DataFrame.Describe alternatives you've considered As discussed with @jepegit, this can be done in a more correct way by fitting a function to the relaxation potential curve and extrapolating the fitted function to find the internal resistance. However, this is a much more complex task and probably hard to automate properly?
Additional context The
_ir_to_summary()
-method of theCellpyCell
-class assigns internal resistances, as I've understood from @jepegit based on values generated by the Maccor-cyclers. I guess this could be a good place to implement the logic for this?