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

1D compositional case without wells #1265

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GitPaean
Copy link
Member

preparing for the regression test for flowexp_comp.

The results for this case have been validated.

@GitPaean GitPaean requested a review from bska December 16, 2024 10:26
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.

Thanks a lot for the contribution. Other than a few minor issues I think this looks good. The one thing I'd prefer we amend before merging is the SUMMARY section. We really should report some production curves/summary vectors in order for the case to be useful as a regression test.


-- This is a 1D, pressure-driven CO₂ flooding example involving three components and two phases.
-- The components are CO₂, methane, and decane.
-- The first and last cells are assigned very large pore volumes to emulate a source and sink,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of saying "The first and last cells..." we could say "Cells (1,1,1) and (30,1,1)..." in order to be very explicit about the coordinates?

Comment on lines 58 to 61
TOPS
30*0
/
Copy link
Member

Choose a reason for hiding this comment

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

That's very shallow. Is there any density variation by depth? If so, how much do the simulation results change if the top of the formation is (much) deeper?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a 1D case and flat. I do not think the depth matters here.

Comment on lines 186 to 187
--BPR
--/

Copy link
Member

Choose a reason for hiding this comment

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

Is the goal of this case to become part of the simulator's regression test suite? If so, we should be reporting at least some production curves here.

--'PRESSURE' /

RPTRST
BASIC = 1
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if this was BASIC=2 instead. BASIC=1 and BASIC=2 do have the same effect here, since we're using UNIFOUT, but BASIC=2 is more explicit about that.

Comment on lines 48 to 57
DX
30*10
/
DY
30*30
/
DZ
30*1
/
Copy link
Member

Choose a reason for hiding this comment

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

I generally prefer the tensor product form for this, i.e.

DXV
  30*10
/
DYV
  10
/
DZV
 1
/

since that's less ambiguous.

@GitPaean
Copy link
Member Author

GitPaean commented Dec 16, 2024

Thanks a lot for the contribution. Other than a few minor issues I think this looks good. The one thing I'd prefer we amend before merging is the SUMMARY section. We really should report some production curves/summary vectors in order for the case to be useful as a regression test.

Since there is not wells involved yet for this case, it is mostly for UNRST file result checking.

Maybe I can check how the block summary keywords or field work for the summary output yet. Will report back.

@GitPaean
Copy link
Member Author

Have not managed to update for summary output yet.

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.

2 participants