Skip to content

feat: Perforation Status #3587

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

Open
wants to merge 50 commits into
base: develop
Choose a base branch
from

Conversation

tjb-ltk
Copy link
Contributor

@tjb-ltk tjb-ltk commented Mar 7, 2025

  1. Assign perforations an open/closed state from user specified time vs status table
  2. Dt selection includes perforation table time
  3. Calculations for inactive perforations will be skipped, but linear solver Awr and Arw arrays are still allocated.
    • this could be fixed in different PR
  4. Well element state introduced to allow closing/opening of segments
    • an element is open if it contains an open perforation, or an upstream element is open
    • Aww = I for closed element entries
  5. 4 integrated tests introduced to test vertical well where perforations are
    • plugged from bottom to top and top to bottom
    • opened from bottom to top and top to bottom

@tjb-ltk tjb-ltk changed the title Feat : Perforation Status feat : Perforation Status Mar 7, 2025
@tjb-ltk tjb-ltk marked this pull request as draft March 7, 2025 19:10
@tjb-ltk tjb-ltk requested review from corbett5 and MelReyCG as code owners May 2, 2025 16:48
Copy link
Contributor

@dkachuma dkachuma 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 to me.

I have a more paradigm question: We are asking the user to provide the perforation status as a 2D array and then we're using this to internally generate a table. Wouldn't it be simpler to just ask the user to provide the table? This would facilitate:

  • Very long and complex open/shut sequences can be relegated to a text file instead of clattering the xml.
  • If the user wants to control multiple perforations in the same way they can define this in one table.

@tjb-ltk
Copy link
Contributor Author

tjb-ltk commented May 5, 2025

Looks good to me.

I have a more paradigm question: We are asking the user to provide the perforation status as a 2D array and then we're using this to internally generate a table. Wouldn't it be simpler to just ask the user to provide the table? This would facilitate:

  • Very long and complex open/shut sequences can be relegated to a text file instead of clattering the xml.
  • If the user wants to control multiple perforations in the same way they can define this in one table.

I'm not aware of the workflows used by the environment developers for GEOS... I assumed

  1. when the perforation info for xml is being generated the perf status is also available.
  2. inlining the status info in the Perf object would avoid one level of indirection (the well model generates the table function, not user).
  3. the timing of open/close is/could be different for perfs. so one coordinate for all perfs isn't ideal and each perf needs its own table

Are you referring to something like below, I can see it has advantages to allow using a separate file , which is fairly straightforward to make the changes...

<InternalWell
name = "well1"
<Perforation
name="perf1"
statusTable="well1_perf1_table"
..

Copy link
Collaborator

@CusiniM CusiniM left a comment

Choose a reason for hiding this comment

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

A few small questions and I think I think that the the way these global status arrays are used deserves some broader discussion to at least generalize this patter because it's different from anything else in geos.
@rrsettgast should have a look at this before it goes in.

registerWrapper( viewKeyStruct::perfStatusTableString(), &m_perfStatusTable ).
setInputFlag( InputFlags::OPTIONAL ).
setSizedFromParent( 0 ).
setDescription( "Table defining perforation status as a function of time. If enterned in Functions section, the name must be the same as the perforation name" );
Copy link
Collaborator

Choose a reason for hiding this comment

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

which name must be the same? the name of the function must match the name of the perforation? We should have an attribute to define the function associated to each perf so that multiple perfs can use the same function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

submitted changes to allow user defined perf status table names

// Setup perforation status function
FunctionManager & functionManager = FunctionManager::getInstance();

if( !functionManager.hasGroup< TableFunction >( getName() ) )
Copy link
Collaborator

Choose a reason for hiding this comment

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

here you could pass the name of the function instead of using the perf name and requiring the function name to be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

submitted changes to allow user defined perf status table names

string_array m_perfName;

/// Local perforation's status
array1d< localIndex > m_localPerfStatus;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the local perf status?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

local means the mesh partition contains that perforation, status means open or closed. Better comment added

Comment on lines 92 to +98
if( perforationData->globalToLocalMap().count( iperfGlobal ) > 0 )
{
elemStatusGlobal[iwelemGlobal] |= WellElementSubRegion::WellElemStatus::LOCAL;
elemStatusGlobal[iwelemGlobal] |= WellElementSubRegion::WellElemParallelStatus::LOCAL;
}
else
{
elemStatusGlobal[iwelemGlobal] |= WellElementSubRegion::WellElemStatus::REMOTE;
elemStatusGlobal[iwelemGlobal] |= WellElementSubRegion::WellElemParallelStatus::REMOTE;
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you add some comments? I am not sure I follow what's happening? what's the meaning of remote and local in this context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are defined in header where the struct is defined. Several well elements data structures are mirrored on all cores. Remote/local is used to identify if the element is spatially located on a different ranks mesh partition.

Comment on lines 347 to 351
localMatrix.addToRowBinarySearchUnsorted< parallelDeviceAtomic >( eqnRowIndices[i],
&dofColIndices[0],
&localPerfJacobian[0][0] + 2 * i,
2 );
RAJA::atomicAdd( parallelDeviceAtomic{}, &localRhs[eqnRowIndices[i]], localPerf[i] );
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at this I would say that if the perf is closed there will be a zero on the diagonal but clearly I am missing where the diagonal is being set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the accumulation kernel

arrayView1d< integer const > const elemStatus =subRegion.getLocalWellElementStatus();
array1d< real64 > & mixConnRate = subRegion.getField< fields::well::mixtureConnectionRate >();
localIndex rank_offset = dofManager.rankOffset();
forAll< serialPolicy >( subRegion.size(), [=] GEOS_HOST_DEVICE ( localIndex const ei )
Copy link
Collaborator

Choose a reason for hiding this comment

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

why serialPolicy? this forces it to happen on host.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good eye... changed to parallel

Comment on lines +1242 to +1246
localMatrix.template addToRow< serialAtomic >( rindex,
&cindex,
&unity,
1 );
localRhs[rindex] = 0.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@castelletto1 , @victorapm will this cause issues to the LS? I am guessing that ideally we would like to have a diagonal entry reasonably scaled with the rest of the diagonal entries in the system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already the current behavior and not introduced by this PR. If this needs to be changed it can be addressed in new PR @CusiniM, @castelletto1 , @victorapm

setRTTypeName( rtTypes::CustomTypes::groupNameRef ).
setInputFlag( InputFlags::OPTIONAL ).
setDescription(
"Name of table function defining perforation status as a function of time. If none specified, a table function with be created internally and assigned the same name as the perforation." );
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add precisions / example:

Suggested change
"Name of table function defining perforation status as a function of time. If none specified, a table function with be created internally and assigned the same name as the perforation." );
"Name of table function defining perforation status as a function of time:\n"
"- The first row sets the perforation time coordinates,\n"
"- The following row is a multiplier of the perforation rate (typically, 0.0 to close, 1.0 to open) for each time coordinate.\n"
"If none specified, a table function with be created internally and assigned the same name as the perforation.\n"
"A table that opens the perforation at t=100, and closes it at t=200 could be `{{ 0, 100, 200 },{ 0.0, 1.0, 0.0 }}`" );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks ... good suggestion I've added the description with a slight modification. The intent of perf status is to be an on/off indicator, with potentially other statuses as needed for well actions. The fractional value I think you are referring to would be a PI multiplier field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run code coverage enables running of the code coverage CI jobs ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: ready for review flag: requires rebaseline Requires rebaseline branch in integratedTests type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants