-
Notifications
You must be signed in to change notification settings - Fork 7
Uniaxial equivalent stress amplitude functionality #55
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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.
typo in the file name iniaxial
| from numpy.typing import ArrayLike, NDArray | ||
|
|
||
|
|
||
| def _validate_stress_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.
This function seems to implement DRY principle, but to me seems a bit clunky and hard to extend
reason might be, that it is trying to do multiple things, so it does not have 1 responsibility only, thus it branches in its logic and is harder to fit to use cases.
think it through if it could not be improved
| for compressive-dominated loading conditions. | ||
|
|
||
| """ | ||
| stress_amp_arr, mean_stress_arr = _validate_stress_inputs(stress_amp, mean_stress) |
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 would convert ArrayLike to np.ndarray here and check for positive values
It is more explicit and does not create too much boilerplate code
In this case validate function seems as overkill to me
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 am not saying that we have to do that. Just going through cases of usage of validate function
| "appropriate for compressive-dominated loading conditions." | ||
| ) | ||
|
|
||
| return np.sqrt(stress_amp_arr * (mean_stress_arr + stress_amp_arr)) |
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.
** 0.5 might be more readable than np.sqrt but it does not matter as much, just is more consistent with the mathematical formulas
| ratio = abs_mean / material_param_arr | ||
|
|
||
| if np.any(ratio >= 1.0): | ||
| raise ValueError( |
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.
what about case, where user inputs real values, where some are simply too high, so some results are negative?
Is it okay to disallow user from calculating all values ?
I personally would rather raise warning and exclude it from this validation function
| ("mean_equals_material", False, "Mean stress magnitude.*exceeds or equals"), | ||
| ], | ||
| ) | ||
| def test_validate_stress_inputs_parametrized( |
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.
validate function requires more then it brings value
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 would recommend using test classes to separate different test baste on function tested
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.
Test class than would be more generic, where each test case within the test class repeats for each tested function
class TestFunctioon1:
def test_feature1():
pass
def test_feature2(): ...
# now same thing for class TestFunction2
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 puts a bit more structure into the test file, so it is easier to navigate
also you can run tests based on the test class and more
Pull request following issue #40 and is prepared for other stress based uniaxial fatigue criteria like #44 , #45 and #46.