feat: add stream conditions to compressor v2 #194
Conversation
jsolaas
force-pushed
the
ECALC-136-refactor-time-series-table-data-frame-functionality-in-core-results
branch
from
24cf571
to
222f93f
Compare
jsolaas
force-pushed
the
feat/add-stream-conditions-compressor
branch
2 times, most recently
from
1a09e34
to
470efb2
Compare
| 125.0 | ||
| ] | ||
| }, | ||
| "stream_day_rates": [ |
There was a problem hiding this comment.
Is the reason for calling it stream_day_rates here that it should always be in stream days? I mean it is a TimeSeriesRate, and there is a rate_type inside. Just curious.
There was a problem hiding this comment.
Yes, that was the reason. I think that name came before we used TimeSeriesRate, but I'm not sure. Seems unnecessary now :)
jsolaas
force-pushed
the
ECALC-136-refactor-time-series-table-data-frame-functionality-in-core-results
branch
2 times, most recently
from
d45765d
to
a1e7740
Compare
jsolaas
force-pushed
the
feat/add-stream-conditions-compressor
branch
from
470efb2
to
27cb608
Compare
jsolaas
force-pushed
the
ECALC-136-refactor-time-series-table-data-frame-functionality-in-core-results
branch
2 times, most recently
from
c22c718
to
da6ae59
Compare
jsolaas
force-pushed
the
feat/add-stream-conditions-compressor
branch
from
27cb608
to
4acc6a2
Compare
jsolaas
force-pushed
the
feat/add-stream-conditions-compressor
branch
2 times, most recently
from
9d79de0
to
9e703db
Compare
Also expanded merge to handle recursive objects and lists.
jsolaas
force-pushed
the
feat/add-stream-conditions-compressor
branch
from
9e703db
to
cf23f35
Compare
| pressure=operational_settings.inlet_pressure, | ||
| ), | ||
| ), | ||
| Stage( |
There was a problem hiding this comment.
Should be easy to extend this list with more stages from models.
There was a problem hiding this comment.
I like the start to more structured add the different stage/phases etc of the stream that you are adding though :)
| class StreamCondition(BaseModel): | ||
| class Config: | ||
| extra = Extra.forbid | ||
| alias_generator = to_camel_case | ||
| allow_population_by_field_name = True | ||
|
|
||
| rate: TimeSeriesRate | ||
| pressure: TimeSeriesFloat | ||
| fluid_density: TimeSeriesFloat = None | ||
|
|
||
|
|
||
| class Stage(BaseModel): | ||
| class Config: | ||
| extra = Extra.forbid | ||
| alias_generator = to_camel_case | ||
| allow_population_by_field_name = True | ||
|
|
||
| name: Literal["inlet", "before_choke", "outlet"] | ||
| stream_condition: StreamCondition |
There was a problem hiding this comment.
Let me know if we have something similar already in models
| try: | ||
| compressor_system_yaml = YamlCompressorSystem(**data) |
There was a problem hiding this comment.
Improves error message when the yaml is incorrect
| allow_population_by_field_name = True | ||
|
|
||
| rate: TimeSeriesRate | ||
| pressure: TimeSeriesFloat |
There was a problem hiding this comment.
liker denne, tenkte at her må vi spesifisere input og output stream. mye likt der, men trykke har jo naturlig nok forandra seg...og det kan andre ting også gjøre :) blir bra dette...og godt utgangspunkt for diskusjon :)
| from pydantic import BaseModel, Extra | ||
|
|
||
|
|
||
| class StreamCondition(BaseModel): |
There was a problem hiding this comment.
I guess usage here is in consumer system (only), but the entity should be called Stream, and it is the Stream that should be used in consumer system imo
There was a problem hiding this comment.
Only consumer system now, but I'm hoping/expecting this to be used for all new components, also in models if we want to introduce TimeSeries there..
It's used in compressor (and when this is approved I will add it to pump) so when we add a pump and compressor v2 (outside system) it will be used there also
|
|
||
| rate: TimeSeriesRate | ||
| pressure: TimeSeriesFloat | ||
| fluid_density: TimeSeriesFloat = None |
There was a problem hiding this comment.
Is this for pump only? Or both compressor and pump? May easily lead to confusion if there is a difference. Not sure if it makes sense with a Fluid and FluidProperties for pump, as it is simpler..but may be that it is the same...or if we should call it Fluid wrt compressors since it is "mostly gas"...to be discussed
There was a problem hiding this comment.
I guess we could have different types of streams, where pump expects a certain type, and compressor another. But I'm hoping we can find a common interface, where the stream itself can handle density etc. That means using streams as input though, so not yet.
But I agree, to be discussed.
| fluid_density: TimeSeriesFloat = None | ||
|
|
||
|
|
||
| class Stage(BaseModel): |
There was a problem hiding this comment.
Name easily confused with Compressor(Train)Stage, but somehow related (I guess). To be discussed. Not sure what is the name being used to define the stage/phase/step/ that the stream goes through...in the system...
There was a problem hiding this comment.
CompressorStage, ASVStage, ChokeStage. I was thinking the common name for all of these were stage, and that is what this represents, so stream is only one of the properties on a stage, might be the only one.. we'll see
| @@ -4,6 +4,7 @@ | |||
| from datetime import datetime | |||
| from typing import Any, Dict, List, Optional, Union | |||
|
|
|||
| from libecalc.common.stream import Stage | |||
There was a problem hiding this comment.
This should go to core (now) or domain (later?) project package though, imo. But we will need a representation in the dto/application layer as well wrt input/output I guess...
There was a problem hiding this comment.
Yes, core/domain. Hopefully dto will be allowed to use it from core. It isn't all that clear to me anymore why we need a dto layer..We have a yaml layer, and can convert to something else there, why do we need a layer between yaml and core? I'm sure there is a reason, and I'm thinking it has something to do with evaluated vs not evaluated expressions..
It was introduced to allow changes, but in v2 we can make changes in core. I dunno
Also expanded merge to handle recursive objects and lists.