(ASIM-4459) Add equipment list for annulus description #210

dettmerramon · 2021-11-26T18:36:04Z

No description provided.

codecov · 2021-11-29T15:45:10Z

Codecov Report

Merging #210 (3e945d4) into rb-ASIM-v1.10.0 (efab4f8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           rb-ASIM-v1.10.0     #210   +/-   ##
================================================
  Coverage            99.90%   99.90%           
================================================
  Files                   42       42           
  Lines                 4333     4344   +11     
================================================
+ Hits                  4329     4340   +11     
  Misses                   4        4

haensch-mauricio · 2021-12-01T16:36:08Z

tests/common_testing/alfasim_sdk_common_testing/filled_case_descriptions.py

@@ -424,7 +424,9 @@
    pvt_model="gavea",
    top_node="mass_source_node",
    initial_conditions=INITIAL_CONDITIONS_DESCRIPTION,
-    gas_lift_valve_equipment={"My gas-lift valve": GAS_LIST_VALVE_DESCRIPTION},
+    equipment=case_description.AnnulusEquipmentDescription(


I think it is better to use the existing description here.

Suggested change

equipment=case_description.AnnulusEquipmentDescription(

equipment=ANNULUS_EQUIPMENT_DESCRIPTION,

prusse-martin · 2021-12-01T20:08:36Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

@@ -1492,7 +1492,7 @@ def load_annulus_description(
        "pvt_model": load_value,
        "top_node": load_value,
        "initial_conditions": load_initial_conditions_description,
-        "gas_lift_valve_equipment": load_gas_lift_valve_equipment_description,
+        "equipment": load_annulus_equipment_description,


Did you test the auto loaders?

by auto loaders do you mean these loaders (load_annulus_equipment_...)? If so some tests already cover it

See #210 (comment)

Maybe using directly load_instance.

prusse-martin · 2021-12-01T20:17:15Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

+        return update_multi_input_flags(document, item_description)
+
+    return {
+        key.data: generate_leak_equipment_description(


Food for thought.
I ask myself why DescriptionDocument do not have an items method.

@attr.s class DescriptionDocument: ... def items(self): for key, value in self.content.items(): yield key, type(self)(value, self.file_path)

Probably will be expected to have other helper methods.

prusse-martin · 2021-12-01T20:21:02Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

+        "gas_lift_valves": load_gas_lift_valve_equipment_description,
+        "leaks": load_leak_equipment_description,
+    }
+    return _generate_description(


Did you try to use auto loaders?

Maybe using directly load_instance.
I would expect all case descriptions that only use those attrib_* or are plain values to be loadeble by auto loaders.
I will like to see this file with as lest code possible like case_to_alfacase.

prusse-martin · 2021-12-01T20:23:08Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

@@ -1742,6 +1762,7 @@ def load_equipment_description(
        "reservoir_inflows": load_reservoir_inflow_equipment_description,
        "heat_sources": load_heat_source_equipment_description,
        "compressors": load_compressor_equipment_description,
+        "leaks": load_leak_equipment_description,


Did you try to use auto loaders?

yup, tried, it worked in some places, see the most recent commits
Wouldnt be a good idea to create a tech debit and update everything that is possible?

prusse-martin · 2021-12-09T17:25:38Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

+    Return a load  instance function pre-populate with the class_. @ramon change the docstring
+    """
+
+    def _get_dict_of_loader(alfacase_content: DescriptionDocument, class_: Type[T]) -> T:


Rename _get_dict_of_loader -> load_dict_of_instance an move it to global scope.

prusse-martin · 2021-12-09T17:27:56Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

@@ -137,8 +125,25 @@ def get_instance_loader(*, class_: type) -> Callable:
    return partial(load_instance, class_=class_)


+@lru_cache(maxsize=None)
+def get_dict_of_loader(*, class_: type) -> Callable:


Suggested change

def get_dict_of_loader(*, class_: type) -> Callable:

def get_dict_of_instance_loader(*, class_: type) -> Callable:

There is get_dict_of_arrays_loader and get_curve_dict_loader. My bad using inconsistent names.

prusse-martin · 2021-12-09T17:29:42Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

 def get_case_description_attribute_loader_dict(
-    class_: Any, explicit_loaders: Optional[Dict[str, Callable]] = None
+    class_: Any, explicit_loaders: Optional[Dict[str, Callable]]=None


Did this pass by precommit check?
There is a lot of this.

hmm, I didn't thought that was why it didnt pass the precommit check, but I'll revert it just to be sure

prusse-martin · 2021-12-09T17:31:31Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

@@ -1492,7 +1498,7 @@ def load_annulus_description(
        "pvt_model": load_value,
        "top_node": load_value,
        "initial_conditions": load_initial_conditions_description,
-        "gas_lift_valve_equipment": load_gas_lift_valve_equipment_description,
+        "equipment": partial(load_instance, class_=case_description.AnnulusEquipmentDescription),


Suggested change

"equipment": partial(load_instance, class_=case_description.AnnulusEquipmentDescription),

"equipment": get_instance_loader(class_=case_description.AnnulusEquipmentDescription),

If not using auto loaders at least keep it simple as possible.

isn't that "using auto loaders" thou?

it uses part of the autoloaders infra.
Those dicts are explicit loaders:

explicit_loaders_for_DecriptionType = { "attr_name_FOO": load_vale, "attr_name_BAR": load_BAR_description, "attr_name_XXX": get_instance_loader(class_=case_description.XXXDescription), }

could be written as:

explicit_loaders = { "attr_name_BAR": load_BAR_description, } loaders = get_case_description_attribute_loader_dict(DecriptionType, explicit_loaders)

load_value it the default loader when no explicit loader is supplied and no metadata is present.
If "attr_name_BAR" is also declared with have metadata (and the autloader is exits) no explicit loader is required and possibly we can remove load_BAR_description altogether.

I would suguest, first just try

def load_annulus_description( document: DescriptionDocument, ) -> case_description.AnnulusDescription: return load_instance(document, case_description.AnnulusDescription)

If this works you probably can remove load_initial_conditions_description if its no longer used (probably is used by the main pipe loader).

If the full autloader do not work try:

# Change `load_instance` to accept optional explict loaders. def load_instance(alfacase_content: DescriptionDocument, class_: Type[T], explicit_loaders: Optional[Dict[str, Callable]] = None) -> T: """ Create an instance of class_ with the attributes found in alfacase_content. """ alfacase_to_case_description = get_case_description_attribute_loader_dict(class_, explicit_loaders) case_values = to_case_values(alfacase_content, alfacase_to_case_description) item_description = class_(**case_values) return update_multi_input_flags(alfacase_content, item_description) def load_annulus_description( document: DescriptionDocument, ) -> case_description.AnnulusDescription: explicit_loaders= { "initial_conditions": load_initial_conditions_description, } return load_instance(document, case_description.AnnulusDescription, explicit_loaders)

BTW, explicit_loaders are not actually used today. I did add it to help with progressive adoption of the autoloaders infrastructure.
I did use it in the initial port of the loaders already ported.

Update the loader to use the autoloaders infrastructure with explict loaders;

Add necessary metadata and autoloaders support;

When the explict loader dict is empty just delete the function =);

This allow to keep the tests passing while progressively implementing the loaders. Implement 1, run the tests, another, run the test. This makes if easier to track a problem when implementing.

prusse-martin · 2021-12-09T17:33:09Z

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

@@ -1742,6 +1748,7 @@ def load_equipment_description(
        "reservoir_inflows": load_reservoir_inflow_equipment_description,
        "heat_sources": load_heat_source_equipment_description,
        "compressors": load_compressor_equipment_description,
+        "leaks": partial(load_instance, class_=case_description.LeakEquipmentDescription),


Suggested change

"leaks": partial(load_instance, class_=case_description.LeakEquipmentDescription),

"leaks": get_instance_loader(class_=case_description.LeakEquipmentDescription),

prusse-martin · 2021-12-09T17:36:34Z

src/alfasim_sdk/_internal/alfacase/case_description_attributes.py

@@ -402,8 +402,12 @@ def attrib_dict_of(type_: type) -> attr._make._CountingAttr:
    Create a new attr attribute with validator for an atribute that is a dictionary with keys as str (to represent
    the name) and the content of an instance of type_
    """
+    metadata = {"type": "dict_of", "class_": type_}


Suggested change

metadata = {"type": "dict_of", "class_": type_}

metadata = {"type": "dict_of_instance", "class_": type_}

plz =)
Possibly this must be different for values and case description instances, but all occurrences I found use case description instances so lets not over complicate things now.

prusse-martin · 2021-12-14T11:18:49Z

BTW, if pre-commit fails the mergeable check I think it don't even check the formatting.
In other words, I think you will need to solve this conflict, maybe rebasing.

for more information, see https://pre-commit.ci

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py

prusse-martin · 2021-12-16T21:09:58Z

HISTORY.rst

@@ -6,6 +6,11 @@ History
 0.12.0 (unreleased)
 ===================

+* **Breaking Change**: Change in ``AnnulusDescription`` to support different types of annulus equipment. Now ``AnnulusDescription``` has an attribute ``AnnulusEquipmentDescription``, which holds a dict that can contain multiple different equipment types, for which the current available options are:
+
+  - ``LeakEquipmentDescription``;


@oliveira-mauricio
Don't you think we should to start to mention explicitly how this affects alfacase files in the history?

I agree. 👍

Like an example? I guess it's ok. I'm a bit afraid it can get too big, but let's give it a try.

Maybe something like this?

Here is an example of how to update previous .alfacase files with annulus equipment:

Before

annulus: gas_lift_valve_equipment: Gas Lift Valve (Well 1 > Annulus) 1: position: value: 100.0 unit: m ... # Other properties

After

annulus: equipment: gas_lift_valves: Gas Lift Valve (Well 1 > Annulus) 1: position: value: 100.0 unit: m ... # Other properties

And maybe add that on the previous release entry too?

Here is an example of how to update previous .alfacase files with trends:

Before

trends: - curve_names: - timestep location: not_defined - curve_names: - flow pattern - pressure element_name: Conn 1 ... # Other properties

After

trends: positional_pipe_trends: - curve_names: - flow pattern - pressure element_name: Conn 1 ... # Other properties overall_pipe_trends: [] global_trends: - curve_names: - timestep equipment_trends: [] separator_trends: []

prusse-martin · 2021-12-16T21:12:44Z

src/alfasim_sdk/_internal/alfacase/case_description.py


-    pressure_boosts: Array = attr.ib(
-        default=Array(
+    pressure_boosts = attrib_array(Array(


I would expect black to nag about this, but OK.

You are right, how odd... 🤔

is black used for the alfasim-sdk tho? It never asked me to use it regardless of how "unformatted" the code seemed

It is:

https://github.com/ESSS/alfasim-sdk/blob/fb-ASIM-4459-equipment-list-annulus-description/.pre-commit-config.yaml#L17-L24

Locally you need to run pre-commit install once, and from that point on, every commit will run the pre-commit checks (which includes black).

But regardless we use pre-commit.ci, which runs a pre-commit job here which would automatically format your code. That's why that's curious.

To be clear: don't worry about it, we were just wondering, there's no action to be taken on your side.

yup, I figured that, since the jobs passed. I just find it odd, since black complains all the time with my commits fo other reps, but never had a problem with the sdk.

Ahh it might be that pre-commit.ci pushes commits fixing the checks when possible. 😉

Mystery solved

# fmt: off @attr.s(frozen=True, slots=True, kw_only=True) class TablePumpDescription: ... pressure_boosts = attrib_array(Array( [0.0] * 12 +[ 12.0, 10.0, 9.0, 7.5, 5.0, 0.0, 10.0, 9.0, 8.0, 6.0, 3.5, 0.0, 14.0, 12.0, 10.0, 8.0, 5.5, 0.0, 13.5, 11.2, 9.5, 7.6, 5.2, 0.0, ], 'bar', ) ) ... # fmt: on

Not a request for a change now.

If we move the fmt: off/on directive to inside the big array it is restricted to that structure.

black playgound example

I would recomend doing

pressure_boosts = attrib_array( Array( [0.0] * 12 + [ # fmt: off 12.0, 10.0, 9.0, 7.5, 5.0, 0.0, 10.0, 9.0, 8.0, 6.0, 3.5, 0.0, 14.0, 12.0, 10.0, 8.0, 5.5, 0.0, 13.5, 11.2, 9.5, 7.6, 5.2, 0.0, # fmt: on ], "bar", ) )

To avoid problems in the future.

for more information, see https://pre-commit.ci

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch 4 times, most recently from 3587aa6 to cebf988 Compare November 28, 2021 21:06

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch 5 times, most recently from 20976ab to b42c565 Compare November 30, 2021 14:26

dettmerramon requested review from haensch-mauricio, prusse-martin and nicoddemus December 1, 2021 16:26

haensch-mauricio reviewed Dec 1, 2021

View reviewed changes

prusse-martin requested changes Dec 1, 2021

View reviewed changes

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch 2 times, most recently from 7ccfd27 to 4a62789 Compare December 8, 2021 21:28

dettmerramon changed the base branch from master to rb-ASIM-v1.10.0 December 9, 2021 13:16

prusse-martin requested changes Dec 9, 2021

View reviewed changes

prusse-martin mentioned this pull request Dec 13, 2021

Controller trends #215

Merged

ramon and others added 4 commits December 14, 2021 14:25

Added AnnulusEquipmentDescription for AnnulusDescription

1481565

Change fille case description to run tests

aeea9c2

[pre-commit.ci] auto fixes from pre-commit.com hooks

cc20eaf

for more information, see https://pre-commit.ci

Changed PR to use auto loaders when possible

ca1725d

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch from 4a62789 to 0bb9ae4 Compare December 14, 2021 17:26

haensch-mauricio reviewed Dec 14, 2021

View reviewed changes

src/alfasim_sdk/_internal/alfacase/alfacase_to_case.py Show resolved Hide resolved

Fix for tests after using more of the autoloaders

cab1837

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch 2 times, most recently from 51a7c88 to 6cf9c9a Compare December 15, 2021 17:50

Converted alfacase to case to use autoloaders in some parts

92dc608

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch from 6cf9c9a to 92dc608 Compare December 15, 2021 17:51

prusse-martin mentioned this pull request Dec 16, 2021

Terminate to implement the auto loaders infra #216

Open

3 tasks

prusse-martin approved these changes Dec 16, 2021

View reviewed changes

haensch-mauricio approved these changes Dec 17, 2021

View reviewed changes

nicoddemus approved these changes Dec 20, 2021

View reviewed changes

Updated HISTORY.rst

86778ec

dettmerramon force-pushed the fb-ASIM-4459-equipment-list-annulus-description branch from 0cd1169 to 86778ec Compare December 20, 2021 17:02

[pre-commit.ci] auto fixes from pre-commit.com hooks

3e945d4

for more information, see https://pre-commit.ci

dettmerramon merged commit 837eba4 into rb-ASIM-v1.10.0 Dec 21, 2021

dettmerramon deleted the fb-ASIM-4459-equipment-list-annulus-description branch December 21, 2021 16:04

	equipment=case_description.AnnulusEquipmentDescription(
	equipment=ANNULUS_EQUIPMENT_DESCRIPTION,

	def get_dict_of_loader(*, class_: type) -> Callable:
	def get_dict_of_instance_loader(*, class_: type) -> Callable:

	"equipment": partial(load_instance, class_=case_description.AnnulusEquipmentDescription),
	"equipment": get_instance_loader(class_=case_description.AnnulusEquipmentDescription),

	"leaks": partial(load_instance, class_=case_description.LeakEquipmentDescription),
	"leaks": get_instance_loader(class_=case_description.LeakEquipmentDescription),

	metadata = {"type": "dict_of", "class_": type_}
	metadata = {"type": "dict_of_instance", "class_": type_}

(ASIM-4459) Add equipment list for annulus description #210

(ASIM-4459) Add equipment list for annulus description #210

Conversation

dettmerramon commented Nov 26, 2021

codecov bot commented Nov 29, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dettmerramon Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusse-martin Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

prusse-martin Dec 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusse-martin commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haensch-mauricio Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haensch-mauricio Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

nicoddemus Dec 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prusse-martin Dec 20, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 29, 2021 •

edited

Loading

dettmerramon Dec 8, 2021 •

edited

Loading

prusse-martin Dec 13, 2021 •

edited

Loading

prusse-martin Dec 13, 2021 •

edited

Loading

haensch-mauricio Dec 17, 2021 •

edited

Loading

haensch-mauricio Dec 17, 2021 •

edited

Loading

nicoddemus Dec 17, 2021 •

edited

Loading

nicoddemus Dec 17, 2021 •

edited

Loading

prusse-martin Dec 20, 2021 •

edited

Loading