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

feat: Inj_Valve_fix #113

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: Inj_Valve_fix #113

wants to merge 1 commit into from

Conversation

cutn94
Copy link

@cutn94 cutn94 commented Jun 20, 2024

Description

feat: Injection valve second run

Why

Add injection valve and fix commit issue #112

How

How the change is implemented (if it's not self evident in the diff)

Close: #111

Checklist:

Before submitting this PR, please make sure:

  • I am complying with the contributing doc
  • My code builds clean without any errors or warnings
  • I have added/extended tests that prove my fix is effective or that my feature works
  • I have made corresponding changes to the documentation, and it builds correctly

@cutn94 cutn94 changed the title Inj_Valve_fix Feat: Inj_Valve_fix Jun 20, 2024
@cutn94 cutn94 changed the title Feat: Inj_Valve_fix feat: Inj_Valve_fix Jun 20, 2024
Copy link
Contributor

@TorbjornWilson TorbjornWilson left a comment

Choose a reason for hiding this comment

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

Some comments, want some changes and a review from somebody else.

@@ -635,6 +635,10 @@ def get_device(df_well: pd.DataFrame, df_device: pd.DataFrame, device_type: Devi
# rescale the Cv
# because no scaling factor in WSEGVALV
df_well[Headers.CV_DAR] = -df_well[Headers.CV_DAR] / df_well[Headers.SCALING_FACTOR]
elif device_type == "INJV":
Copy link
Contributor

Choose a reason for hiding this comment

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

Make oneliner, ie:
'# rescale the CV, because there are no scaling factor in WSEGVALV.'
This can be done in all occasions or replace the three to one time at the top.

@@ -126,6 +126,12 @@ class Headers:
GHF_LCF_DAR = "GHF_LCF_DAR"
GHF_HCF_DAR = "GHF_HCF_DAR"

CV_INJV = "CV_INJV"
AC_PRIMARY = "AC_PRIMARY"
AC_SECONDARY = "AC_SECONDARY"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of this names, but it's ok for now

@@ -132,6 +132,17 @@ def __init__(
{"-" * 100}{self.newline1}"""

self.print_wsegdarinit = self.print_wsegdar
self.print_wseginjv = f"""\
{'-' * 100}
-- This is how we model Injection Valve technology using sets of ACTIONX keywords.
Copy link
Contributor

Choose a reason for hiding this comment

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

repeating information.
Should the line maybe be?
-- The segment dP curves changes according to the segment water rate and segment -
-- pressure drop at downhole condition.

instead?

def make_wseginjv(self) -> None:
"""Print WSEGINJV to file."""
if self.df_wseginjv.shape[0] > 0:
self.print_wseginjv += po.print_wseginjv(self.df_wseginjv, self.iwell + 1) + "\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not super fan how this is written, maybe a TODO on refactoring this selv update stuff?
But that is not relevant for this PR.

@@ -412,7 +430,12 @@ def fix_printing(self) -> None:
self.print_wsegdar = ""
else:
self.print_wsegdar += self.newline1
# if no DAR then dont print
# if no Injection Valve then dont print
Copy link
Contributor

Choose a reason for hiding this comment

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

"If there are no injection valve dont print update the print statement."
Maybe reduntant comment,

well_name = df_wseginjv[Headers.WELL].iloc[idx]
action += f" ASSIGN SUVTRIG {well_name} {segment_number} 0 /\n"
action += "/\n\n"
iaction = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the number 1 directly?

water_segment_rate_cutoff = df_wseginjv[Headers.WR_CF_INJV].iloc[idx]
pressure_drop_cutoff = df_wseginjv[Headers.PRD_CF_INJV].iloc[idx]

iaction = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

should not be set in every loop

print_df = df_wseginjv[df_wseginjv[Headers.SEG] == segment_number]
print_df = print_df[header[iaction]] # type: ignore
header_string = Keywords.WSEGVALV + "\n--"
for item in header[iaction]:
Copy link
Contributor

Choose a reason for hiding this comment

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

this loop seems unnecessary

print_df += f"\nUDQ\n ASSIGN SUVTRIG {well_name} {segment_number} 1 /\n/\n"
action += print_df + "\nENDACTIO\n\n"

return action
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is a bit long

Headers.PRD_CF_INJV,
]

# Fix table format
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Fix table format
# Fix table format.

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.

Implement model for injection valve
2 participants