implement new features, fix small bugs#3
Conversation
SevenOfNinePE
left a comment
SevenOfNinePE
left a comment
There was a problem hiding this comment.
You add files dvdt.py and current_capability.py. In my opinion a class could structure it in a better way. On the other hand you introduce the class 'FatigueCurve' and 'SemiLogCurve', but implement the main algorithm part within the function 'voltage_rating_due_to_lifetime'. This is confusing.
However I do not find a point, to request changes.
| :type capacitor_type_list: CapacitorType | ||
| """ | ||
| c_db, c_thermal, c_derating = load_capacitors(capacitor_type_list) | ||
| def download_esr_csv_files() -> None: |
There was a problem hiding this comment.
No input parameter is provided anymore. Which type will be downloaded now?
I guess these in the 'FOIL_CAPACITOR_SERIES_NAME_LIST'. But this approach establish a dependency to this list, which makes this function less universal.
There was a problem hiding this comment.
fixed by adding a list of capacitor series names to download.
If no list is provided, the default list is used
| from cst.cst_dataclasses import LifetimeDerating | ||
|
|
||
| class FatigueCurve: | ||
| """ |
There was a problem hiding this comment.
Class consists only of members, but no methods. Why not dataclass?
There was a problem hiding this comment.
has been fixed and is a function from now on
| __call__ = get_voltage | ||
|
|
||
| def voltage_rating_due_to_lifetime(target_lifetime: float, operating_temperature: float, voltage_rating: float, | ||
| lt_dto_list: list[LifetimeDerating], is_plot: bool = False) -> float: |
There was a problem hiding this comment.
This function uses the class 'SemiLogCurve' to get information. The used class is very simple, so that it can also be substitute by a function. What is the reason for using a class? The main function 'voltage_rating_due_to_lifetime' could be also a method of the class. In this case a class makes more sense.
There was a problem hiding this comment.
changed to a function
| voltage = curve.get_voltage(target_lifetime) | ||
|
|
||
| if is_plot: | ||
| plt.semilogx(df_lower["lifetime"], df_lower["voltage"], label=f"{temperature_lower} °C") |
There was a problem hiding this comment.
I would separate the plot from the function.
Background: The display of the calculations can be implemented by the user on his need. Here in case of 'is_plot' the calculation will be interrupted by the plot. But is_plot are used also at other locations, so that also there a plot is visible. Plotting is not bad, but I like separation of calculation and display results.
There was a problem hiding this comment.
it is more for debugging
2 participants
Implemented new features:
fix bugs: