chore: handle requested pressures for compressor systems #215

frodehk · 2023-09-28T11:04:12Z

Why is this pull request needed?

Extracting requested inlet- and outlet pressure does not work correctly for compressor systems:
When using SUCTION_PRESSURES or DISCHARGE_PRESSURES there are one pressure per component in the system. This list of pressures is not captured and the pressures are set to nan, and then nan to num (0).

What does this pull request change?

Update get_requested_compressor_pressures in graph_result, to select correct pressures for compressors in system
Ensure correct operational setting is used for the compressors in the system

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-252?atlOrigin=eyJpIjoiYTJiZGUwNmY5MjlhNGMwMjg5YTc1Y2FiNTY0ZTkxNDQiLCJwIjoiaiJ9

jsolaas

Can we drop this and say requested pressures are good enough until v2? Wouldn't this cause confusion? If I understand it correctly this is not necessarily correct?

frodehk · 2023-09-28T13:14:02Z

Can we drop this and say requested pressures are good enough until v2? Wouldn't this cause confusion? If I understand it correctly this is not necessarily correct?

No problem for me. Maybe disable requested pressure plots for systems then. Right now e.g. requested_inlet_pressures show up as 0 if e.g. SUCTION_PRESSURES are given (one pressure per component).

TeeeJay · 2023-09-29T08:56:41Z

Did you discuss this today? Should we just drop it for now? It just turns out to be "full of holes" because of all the different edge cases?

frodehk · 2023-09-29T12:39:33Z

@TeeeJay and @jsolaas : Discussed with @HeddaSvendsen, she reminded me of the operational_settings_used. The code is now updated, and it should extract the correct requested pressures now - taking into account the relevant operational setting used, and picking the correct compressor in the system (if different pressures are specified for individual system components within a temporal model).

Have checked the model (advanced model) that was used to flag the problem with zero requested inlet pressure, it now runs with expected output (for the requested pressures).

TeeeJay · 2023-10-03T10:14:00Z

src/libecalc/core/graph_result.py

@@ -215,50 +216,90 @@ def _parse_emissions(emissions: Dict[str, EmissionResult]) -> Dict[str, libecalc
            for key in emissions
        }

+    @staticmethod
+    def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt):


Suggested change

def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt):

def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt) -> int:

TeeeJay · 2023-10-03T10:15:39Z

src/libecalc/core/graph_result.py

+    def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt):
+        timestep_index = operational_settings_used.timesteps.index(timestep)
+
+        operational_setting_index = operational_settings_used.values[timestep_index]


I would prefer to use a different name. This is easily confused with the index in the list. How about operational_setting_used? Or perhaps you have sth better?

I like operational_setting_used, as it refers to one entry in the in the time series operational_settings_used, or?

TeeeJay · 2023-10-03T10:24:28Z

This is getting overly complicated. Not your fault, it is just very difficult to get this good and right. I am afraid that we do not have enough overview of edge cases, where this will work and not etc, and whether the user can always trust the results? If we are to merge this, we should make sure that the code is isolated (doesnt break anything else), and possibly give a warning about this being experimental (using Feature.experimental, in case it fails?) which also gives a warning....

Also, we do not have any other tests than the standard tests to test this.

What do you think?

frodehk · 2023-10-03T10:31:48Z

I agree that it is complicated, and difficult to control edge cases - with few tests really checking. On the other hand, the code that is already merged in is trying to handle the requested pressures for systems in a wrong way - and this will not make it worse I think. Is it an idea to create some targeted tests for this, especially for operational settings in compressor systems - and checking for different pressures for different compressors in the system etc.?

TeeeJay · 2023-10-03T10:45:54Z

I agree that it is complicated, and difficult to control edge cases - with few tests really checking. On the other hand, the code that is already merged in is trying to handle the requested pressures for systems in a wrong way - and this will not make it worse I think. Is it an idea to create some targeted tests for this, especially for operational settings in compressor systems - and checking for different pressures for different compressors in the system etc.?

I would say it is better to wrap with Feature.experimental(), for information + handle gracefully in case of error. If we get any more complications with this, we might want to remove it. It will be handled better in the future in any case.

TeeeJay · 2023-10-03T10:46:11Z

So, also, we need to patch it then...

jsolaas

Looks good

* chore: handle requested pressures for compressor systems (cherry picked from commit 6b05439)

* chore: handle requested pressures for compressor systems (#215) (cherry picked from commit 6b05439) * chore: move feature experimental to main method for requested pressures (#230) (cherry picked from commit 00ad854)

* chore: handle requested pressures for compressor systems

frodehk requested a review from a team as a code owner September 28, 2023 11:04

frodehk self-assigned this Sep 28, 2023

frodehk force-pushed the ECALC-252-handle-requested-pressures-for-compressor-systems branch from 709a82d to ddec1b9 Compare September 28, 2023 11:08

jsolaas reviewed Sep 28, 2023

View reviewed changes

frodehk force-pushed the ECALC-252-handle-requested-pressures-for-compressor-systems branch from 53a9339 to 367915a Compare September 29, 2023 12:45

TeeeJay reviewed Oct 3, 2023

View reviewed changes

frodehk added 5 commits October 3, 2023 14:12

chore: handle requested pressures for compressor systems

180f773

refactor: requested pressure type

47f5bdc

chore: use correct operational settings

265bd11

refactor: change name get operational settings

aa992f7

chore: add feature experimental warning

c34e728

frodehk force-pushed the ECALC-252-handle-requested-pressures-for-compressor-systems branch from 2ebc77b to c34e728 Compare October 3, 2023 12:12

jsolaas approved these changes Oct 4, 2023

View reviewed changes

frodehk merged commit 6b05439 into main Oct 4, 2023
6 checks passed

frodehk deleted the ECALC-252-handle-requested-pressures-for-compressor-systems branch October 4, 2023 09:09

github-actions bot mentioned this pull request Oct 4, 2023

chore(main): release 8.5.0 #206

Merged

frodehk added a commit that referenced this pull request Oct 4, 2023

chore: handle requested pressures for compressor systems (#215)

cdb7f27

* chore: handle requested pressures for compressor systems (cherry picked from commit 6b05439)

equinor-schen pushed a commit that referenced this pull request Nov 22, 2023

chore: handle requested pressures for compressor systems (#215)

3a38fb6

* chore: handle requested pressures for compressor systems

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: handle requested pressures for compressor systems #215

chore: handle requested pressures for compressor systems #215

frodehk commented Sep 28, 2023 •

edited

Loading

jsolaas left a comment

frodehk commented Sep 28, 2023

TeeeJay commented Sep 29, 2023

frodehk commented Sep 29, 2023 •

edited

Loading

TeeeJay Oct 3, 2023

TeeeJay Oct 3, 2023

TeeeJay Oct 3, 2023

frodehk Oct 3, 2023

frodehk Oct 3, 2023

TeeeJay commented Oct 3, 2023

frodehk commented Oct 3, 2023 •

edited

Loading

TeeeJay commented Oct 3, 2023

TeeeJay commented Oct 3, 2023

jsolaas left a comment

	def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt):
	def get_operational_setting_index(timestep: datetime, operational_settings_used: TimeSeriesInt) -> int:

chore: handle requested pressures for compressor systems #215

chore: handle requested pressures for compressor systems #215

Conversation

frodehk commented Sep 28, 2023 • edited Loading

Why is this pull request needed?

What does this pull request change?

Issues related to this change:

jsolaas left a comment

Choose a reason for hiding this comment

frodehk commented Sep 28, 2023

TeeeJay commented Sep 29, 2023

frodehk commented Sep 29, 2023 • edited Loading

TeeeJay Oct 3, 2023

Choose a reason for hiding this comment

TeeeJay Oct 3, 2023

Choose a reason for hiding this comment

TeeeJay Oct 3, 2023

Choose a reason for hiding this comment

frodehk Oct 3, 2023

Choose a reason for hiding this comment

frodehk Oct 3, 2023

Choose a reason for hiding this comment

TeeeJay commented Oct 3, 2023

frodehk commented Oct 3, 2023 • edited Loading

TeeeJay commented Oct 3, 2023

TeeeJay commented Oct 3, 2023

jsolaas left a comment

Choose a reason for hiding this comment

frodehk commented Sep 28, 2023 •

edited

Loading

frodehk commented Sep 29, 2023 •

edited

Loading

frodehk commented Oct 3, 2023 •

edited

Loading