Skip to content

Ensure GUIDRAT is checked when WGRUPCON value is defaulted - #5329

Open
vkip wants to merge 1 commit into
OPM:masterfrom
vkip:guiderate_fix
Open

Ensure GUIDRAT is checked when WGRUPCON value is defaulted#5329
vkip wants to merge 1 commit into
OPM:masterfrom
vkip:guiderate_fix

Conversation

@vkip

@vkip vkip commented Sep 1, 2026

Copy link
Copy Markdown
Member

No description provided.

@vkip vkip added the manual:bugfix This PR is a bug fix and should be noted in the manual label Sep 1, 2026
@vkip
vkip marked this pull request as ready for review September 1, 2026 12:21

@bska bska left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fall-back to GUIDERAT for defaulted WGRUPCON is okay. I am however not very excited about the new getPotential() function. Potential flow rates are supposed to be an internal implementation detail in the GuideRate class. Can you not use potential rates available elsewhere?

@vkip

vkip commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

The fall-back to GUIDERAT for defaulted WGRUPCON is okay. I am however not very excited about the new getPotential() function. Potential flow rates are supposed to be an internal implementation detail in the GuideRate class. Can you not use potential rates available elsewhere?

As far as I know, not without something that seems like unnecessary code duplication. It is the potentials at the times guide rates are updated that I'm after, so appears it would require communicating potentials and storing them somewhere else, with very similar setup as for the current guiderates. If you have other ideas, please feel free to point me in the right direction and I'll have a go.

@bska

bska commented Sep 4, 2026

Copy link
Copy Markdown
Member

It is the potentials at the times guide rates are updated that I'm after

Okay, understood. The contents of GuideRate::potentials are updated on every call to GuideRate::compute(wgname) which, very strictly speaking, could be more frequently than when we actually update the guide rates. The latter depends on the delay setting in item 1 of the GUIDERAT keyword. Depending on your point of view, updating the stored potentials more frequently than the guide rates might even be a bug.

In any case, I really don't like the idea of client code starting to depend on the fact that we have some version of some potential rates stored inside the GuideRate object. I suppose if we give the function a threatening name like latestGuideratePotentialsFor(wgname) then that might abate the risk slightly. Even then, I would really prefer that this coupling not become widespread.

@vkip

vkip commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

It is the potentials at the times guide rates are updated that I'm after

Depending on your point of view, updating the stored potentials more frequently than the guide rates might even be a bug.

Good point, and that can surely be discussed, but for consistency with what's currently being done for entities where no guide rate value is found I think we should stick to returning the most recent potential (was just a bit imprecise above since I didn't think of this issue...)

In any case, I really don't like the idea of client code starting to depend on the fact that we have some version of some potential rates stored inside the GuideRate object. I suppose if we give the function a threatening name like latestGuideratePotentialsFor(wgname) then that might abate the risk slightly.

Another option could be to propagate always_use_potentials with default false to the GuideRate::get function - would that be better?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:bugfix This PR is a bug fix and should be noted in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants