Material Laws - S-N curves #58
Conversation
Vybornak2
left a comment
Vybornak2
left a comment
There was a problem hiding this comment.
some variable names should be adjusted according to nomenclature faber table, that is being created at the time of this review (dec 2025).
| """Abstract base class for stress-life (S-N) curve models.""" | ||
|
|
||
| @abstractmethod | ||
| def stress_amp(self, life: ArrayLike) -> ArrayLike: |
There was a problem hiding this comment.
n_cycles could be more understandable to broader audience?
with variable names adjustment wait until confirmation please
| class WholerPowerLaw(SN_Curve): | ||
| """Wöhler (S-N) curve model using a power law relationship.""" | ||
|
|
||
| def __init__(self, SN_C: float, SN_w: float): |
There was a problem hiding this comment.
SN_C, SN_w are compared to rest of the library shortcuts, which is not desirable
at the architecture meeting longer names such as power_law_coef, power_law_exp has been suggested
| class WholerPowerLaw(SN_Curve): | ||
| """Wöhler (S-N) curve model using a power law relationship.""" | ||
|
|
||
| def __init__(self, SN_C: float, SN_w: float): |
There was a problem hiding this comment.
It should be thought out, if we might want to scale this to multiple dimensions
that would allow assembly of multiple SN curves, without using for cycle
user might want to assess multiple welds for example, utilizing different coefficients
therefore it is quite desirable to vectorize such function, what do you think?
There was a problem hiding this comment.
this would mean that type of both coefs would be ArrayLike
| if np.any(life_array <= 0): | ||
| raise ValueError("life must contain only positive values") | ||
|
|
||
| return np.power((self.SN_C) / life_array, 1 / self.SN_w) |
There was a problem hiding this comment.
unless you need to use additional arguments of np.power, use ** for better readability as both have same performance
| if np.any(stress_amp_array <= 0): | ||
| raise ValueError("stress_amp must contain only positive values") | ||
|
|
||
| return self.SN_C / np.power(stress_amp, self.SN_w) |
There was a problem hiding this comment.
use ** instead of np.power
| derivative_constant = self.A * self.beta * np.power(self.C, self.beta) | ||
|
|
||
| converged = False | ||
| for _ in range(max_iterations): |
There was a problem hiding this comment.
For better understanding I would recommend recording iterations, and add logging that would be enabled via kw param
this would allow user to debug the process
we can discuss this on the meeting (please bring it up, if I don't)
There was a problem hiding this comment.
this could be potentially be implemented in second iteration - issue could be created
two things could take place:
- include logging process
- batch processing for newton method as the data could be potentially vectorized
both things would be nice to have, but might not be necessary immediately
| for _ in range(max_iterations): | ||
| # Calculate function value f(N) = A * (C*(N+B)/(N+C))^β - σ_a | ||
| f_N = ( | ||
| self.A * np.power((self.C * (N + self.B)) / (N + self.C), self.beta) |
There was a problem hiding this comment.
use ** instead of np.power
|
|
||
| # Calculate derivative f'(N) = A*β*C^β*(N+B)^(β-1)*(C-B)/(N+C)^(β+1) | ||
| f_prime_N = derivative_constant * ( | ||
| (np.power(N + self.B, self.beta - 1) * (self.C - self.B)) |
There was a problem hiding this comment.
use ** instead of np.power
| ) | ||
|
|
||
| # Avoid division by zero | ||
| f_prime_N = np.where(np.abs(f_prime_N) < 1e-15, 1e-15, f_prime_N) |
There was a problem hiding this comment.
Could these hardcoded values lead to bug?
Would dtype affect this?
Is this the optimal way how to solve this problem?
| N = N_new | ||
|
|
||
| # Issue warning if Newton solver did not converge | ||
| if not converged: |
There was a problem hiding this comment.
for else pattern can be used here instead
but this is up to you, some may consider it being antipattern for it not being too well known
2 participants
Pull request following Issues #56 and #57 implementing S-N curve models