-
Notifications
You must be signed in to change notification settings - Fork 121
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
Add support for SLAVES keyword #5436
base: master
Are you sure you want to change the base?
Conversation
Since we SLAVES does not do anything at the moment, isn't it better to wait with merging this until we actually supports this feature. For testing you can always set the parsing-strictness=low |
@totto82 Good point. |
I convert this to draft until it is ready for merging |
Rebased |
@blattms I have removed the draft state of this PR, in accordance with #5620 (comment). Can you have a look again at this PR and see if we can get it merged? |
{ | ||
"SLAVES", | ||
{ | ||
{1,{true, [](const std::string& val){ return val.size()<=8;}, "SLAVES(SLAVE_RESERVOIR): Only names of slave reservoirs of up to 8 characters are supported."}}, |
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.
We should check with the engineers whether they really only use 8 or reky on truncation and use the rest for further documentation. I think the commercial simulator just truncates.
@blattms Referring to: #5453 (comment) : then we should use "false" instead of "true" here and in the keyword handlers in opm-common, we should truncate the length if the slave reservoir if it is more than 8 characters long?
I would like to run jennkins here. Would be cool if you would rebase this onto current master. Without that some updated tests might actually fail. |
Just to clarify: This PR is there to simply mark the SLAVE keyword as supported? IMHO, then this PR should actually be the last one in the sequence to support reservoir coupling. Once this is a user can run simulations with coupled reservoirs with the default option. He might expect that such a simulation then works and be surprised that it does not, because the feature is still work in progress. I don't think that having the SLAVE keyword marked as supported is needed for testing. I would simply start flow with |
@blattms this PR should actually be the last one in the sequence to support reservoir coupling. Good point. I put this back into draft state then. |
Depends on upstream OPM/opm-common#4114.
This is a first step to implement reservoir coupling
Currently the
SLAVES
keywords does nothing. The plan is to activate the keyword afterGRUPMAST
andGRUPSLAV
has been implemented. This can hopefully be done usingMPI_Comm_spawn()
from the master, and then MPI communication between master and slaves.