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

Constant PVT Table #416

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

Conversation

caio-ramos
Copy link
Contributor

@caio-ramos caio-ramos commented Jan 2, 2025

Here are the features required to allow https://github.com/ESSS/alfasim/pull/796 work properly.

A new PVT model description is added, so user-defined properties can be stored and used to generate a PVT Table with constant properties.

@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from ac427ae to 175f770 Compare January 2, 2025 13:53
@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from 0962be3 to 79b381b Compare January 2, 2025 13:58
@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from 707e8e7 to eb832ef Compare January 2, 2025 14:26
@caio-ramos caio-ramos changed the title Fb asim 5868 const pvt table model Constant PVT Table Jan 2, 2025
@caio-ramos caio-ramos marked this pull request as ready for review January 2, 2025 15:26
@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from 9243a0b to eb832ef Compare January 2, 2025 19:41
@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from dcbfd0f to 8982476 Compare January 2, 2025 19:54
CHANGELOG.rst Outdated Show resolved Hide resolved
Copy link

@marinhoeng marinhoeng left a comment

Choose a reason for hiding this comment

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

Excellent work @caio-ramos.

Comment on lines +2401 to +2402
oil_specific_heat = attrib_scalar(default=Scalar(4181.3, "J/kg.K"))
water_specific_heat = attrib_scalar(default=Scalar(4181.3, "J/kg.K"))

Choose a reason for hiding this comment

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

Are specific heat values really identical for oil and water?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure. To define these values, I adopted the values ​​already available in the arguments of the create_constant function

Comment on lines +2406 to +2408
gas_oil_surface_tension = attrib_scalar(default=Scalar(7.197e-2, "N/m"))
gas_water_surface_tension = attrib_scalar(default=Scalar(7.197e-2, "N/m"))
oil_water_surface_tension = attrib_scalar(default=Scalar(7.197e-2, "N/m"))

Choose a reason for hiding this comment

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

Is there some motive to consider the same value for surface tension for all the phases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, the reason why these values are here is the same as for oil_specific_heat and water_specific_heat

Comment on lines +19 to +20
:"kW.h/tonUK": kilowatt hours/U.S. ton
:"kW.h/tonUS": kilowatt hours/U.S. ton

Choose a reason for hiding this comment

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

Are descriptions for tonUK and tonUS identical?

Copy link
Contributor Author

@caio-ramos caio-ramos Jan 7, 2025

Choose a reason for hiding this comment

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

These descriptions are defined based on the barril repository. Looking at the specific energy unit database ( barril/units/posc), we can see that they are identical, which I believe is incorrect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created the issue ESSS/barril#165 in the barril repository to report this problem..

Copy link
Member

Choose a reason for hiding this comment

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

FYI we do not look at the repository issues when we plan our sprints, so if you want to see that fixed eventually, you need to create a JIRA issue.

Copy link
Contributor

@eniohayashi eniohayashi left a comment

Choose a reason for hiding this comment

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

Nice work!

CHANGELOG.rst Outdated Show resolved Hide resolved
@caio-ramos caio-ramos force-pushed the fb-ASIM-5868-const-PVT-table-model branch from a020699 to c0d1796 Compare January 6, 2025 19:42
Copy link

sonarqubecloud bot commented Jan 6, 2025

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.

6 participants