Skip to content
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

fixed bug that leads to 1D linear material not behaving properly in simulations #108

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JRao-rgb
Copy link

@JRao-rgb JRao-rgb commented Nov 16, 2023

Current situation

#110
I found out the linear material model is broken when I was testing the backwards compatibility of the leaky vessels code with previous sv input files. I initially thought it was my code that was broken, but the code just didn't handle linear materials properly.

Release Notes

  • Changed the following functions in cvOneDMaterialLinear.cxx: GetArea(), GetPressure(), GetDpDS(). Not sure how the previous maintainer arrived at these forms, but they differ a lot from the implementation in cvOneDMaterialOlufsen.cxx, which doesn't make much sense (since the two models are only different in the EHR computation).
  • Changed the function GetN() in cvOneDMaterialLinear.h from "return 0.0;" to "return N;".
  • Added relevant test cases so that the linear material can be tested when future versions of the code are changed.

Documentation

  • Just a bug fix.

Testing

Please ensure that the PR meets the testing requirements set by GitHub Actions.

  • See above for testing results. The linear material should undergo tests now.

Code of Conduct & Contributing Guidelines

@JRao-rgb
Copy link
Author

@mrp089 should be good to merge! Please take a look when you get time.

@mrp089 mrp089 self-requested a review November 17, 2023 02:32
Copy link
Member

@mrp089 mrp089 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for doing this, @JRao-rgb! Please see my comments and ask me if you have any questions. Congratulations on your first pull request!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check out this file from master branch because there are only formatting changes in here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to do one of these tests with " $Eh/r \ll\infty$ " for Olufsen and Linear material? Then, we should see that they deform similarly. Currently, So_/S would always be close to one, right?

@@ -88,7 +88,9 @@ cvOneDMaterialLinear& cvOneDMaterialLinear::operator=(const cvOneDMaterialLinear
}

void cvOneDMaterialLinear::SetEHR(double ehr_val, double pref_val){
ehr = ehr_val;
ehr = 4.0/3.0*ehr_val; // JR 15/11/23: multiplied EHR by the correct factor (since downstream analysis using EHR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add date and initials (even though people did it before), that's now all stored in git. You can check it out by doing a git blame on a file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants