-
Notifications
You must be signed in to change notification settings - Fork 112
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
Facilitate UDA for WSEGVALV item 4 #3684
Conversation
I guess this begins to address Issue #3493? If so, that's very welcome. |
I wasn't aware of the issue, but yes - intention is to address exactly this. |
Cool! Please don't hesitate to ask if/when you get to the part concerning representing these values in the restart file. |
Thanks, I definitely need input on that - will reach out next week. |
aff059a
to
bf7aa6a
Compare
bf7aa6a
to
cadbcda
Compare
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 think this looks good, except for a possible refinement using the mutable
keyword instead.
double Valve::conCrossArea() const { | ||
return m_con_cross_area; | ||
double Valve::conCrossArea(const std::optional<const ValveUDAEval>& uda_eval_optional) { | ||
m_con_cross_area_value = uda_eval_optional.has_value() ? | ||
uda_eval_optional.value().value(m_con_cross_area, m_udq_default) : | ||
m_con_cross_area.getSI(); |
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.
Maybe instead of making the member function be non-const
we can mark the m_con_cross_area
data member be mutable
instead? That way we can still call conCrossArea()
on a const Valve
object and we might not have to provide read/write access to the Valve
and segment set objects.
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.
Ah, that's much better, thanks. Will update.
jenkins build this please |
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.
Thank you for the updates. This looks good to me now and I'll merge into master.
No description provided.