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

Support WCONHIST/WCONINJ in ACTIONX #4527

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

Conversation

blattms
Copy link
Member

@blattms blattms commented Mar 18, 2025

This is now battle tested by the client.

@blattms
Copy link
Member Author

blattms commented Mar 18, 2025

jenkins build this opm-simulators=6092 please

@blattms blattms marked this pull request as ready for review March 19, 2025 11:10
Copy link
Member

@GitPaean GitPaean left a comment

Choose a reason for hiding this comment

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

It looks like routine change to me.

Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Looks good for the most part, although you're introducing more work than needed in (very) rare cases.

@@ -171,6 +171,7 @@ void handleWCONHIST(HandlerContext& handlerContext)
handlerContext.state().wellgroup_events().addEvent( well_name, ScheduleEvents::REQUEST_OPEN_WELL);
}

handlerContext.affected_well(well_name);
Copy link
Member

Choose a reason for hiding this comment

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

Very strictly speaking this statement should probably be moved up a bit, into the update_well block. If you put it here the simulation layer will be forced to reset the well controls even if the controls don't actually change (rare case).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am rather skeptical about this. The history and bhp limit seems to be set in WellKeywordHandlers.cpp#L134, but update_well stays fasle. It is unclear to me whether true is implied from somewhere else.

Copy link
Member

Choose a reason for hiding this comment

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

The history and bhp limit seems to be set in WellKeywordHandlers.cpp#L134, but update_well stays fasle.

The WellProductionProperties::handleWCONHIST() call on line 134 changes an independent object created on line 103. Whether or not that has any effect at all on the well itself is determined in the Well::updateProduction() call on line 151. If it does, then update_well gets set to true.

@@ -328,6 +329,7 @@ void handleWCONINJH(HandlerContext& handlerContext)
handlerContext.state().wellgroup_events().addEvent( well_name, ScheduleEvents::REQUEST_OPEN_WELL);
}

handlerContext.affected_well(well_name);
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be in the update_well block.

@blattms
Copy link
Member Author

blattms commented Mar 24, 2025

I'll test the update_well approach, too. I did not go done that route intentionally, but let's see.

But I need to do other stuff first and this will be hanging for a few days...

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

3 participants