-
Notifications
You must be signed in to change notification settings - Fork 5
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
chore: add power adjustment constant also for compressor trains with… #136
chore: add power adjustment constant also for compressor trains with… #136
Conversation
355.37085, | ||
346.048583, | ||
358.164004, | ||
356.423481, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change is > 1 MW because there is a 0.05 power loss factor on top
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this pull request not about power adjustment constants? Or power loss factor? In my head thats two different things.
Guess power adjustment constant would here be 1MW?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but there is an additional power loss factor (0.05) in addition to the power adjustment constant (1 mw), which means 1 / 0.95 = 1.05... I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The power adjustment constant is applied to the compressor train result (or other energy function results). The power loss factor is applied to the compressor train result (or other energy function results) before making it a consumer function result. So if the MODEL has a POWER_ADJUSTMENT_CONSTANT and the CONSUMER have a POWERLOSSFACTOR, they will accumulate in the consumer results which is what we look at in the json file here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ok. What about power usage adjustment factor? Is that not supported for compressors?
Would also like a test of this outside "all_energy_usage_models"
@@ -769,15 +769,18 @@ def evaluate_rate_ps_pint_pd( | |||
train_results.append(compressor_train_result) | |||
|
|||
power_mw = np.array([time_step.power_megawatt for time_step in train_results]) | |||
power_mw_adjusted = np.where( | |||
power_mw > 0, power_mw + self.data_transfer_object.energy_usage_adjustment_constant, power_mw |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
power_mw > 0, power_mw + self.data_transfer_object.energy_usage_adjustment_constant, power_mw | |
power_mw > 0, power_mw + self.energy_usage_adjustment_constant, power_mw |
In my opinion I am not fan of the DTO layer not being contained in the initialisation of objects.
Suggest creating 'self.data_transfer_object.energy_usage_adjustment_constant' in the init method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but this has been violated all over the place already ..
355.37085, | ||
346.048583, | ||
358.164004, | ||
356.423481, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this pull request not about power adjustment constants? Or power loss factor? In my head thats two different things.
Guess power adjustment constant would here be 1MW?
@@ -1044,9 +1044,9 @@ | |||
"typ": "STREAM_DAY", | |||
"unit": "MW", | |||
"values": [ | |||
644.62915, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the values decrease...do you know why? was that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as far as I can see they have all 1 MW in PAC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the power capacity margin, which should decrease when the power increase.
Looks good. I also guess that it does make sense to add the power loss factor AFTER the power adjustment constant (PAC), and not before... Should that be documented? |
…interstage pressure chore: update snapshots after power adjustment constant fix for compressor trains with interstage pressure chore: add test for adjust energy usage on multiple streams and pressures compressor trains
e21a039
to
737f94b
Compare
* up-main: (65 commits) chore(docs): fix equations showing twice (equinor#141) chore: fix spelling errors in changelog chore(main): release 8.3.0 chore: add power adjustment constant also for compressor trains with interstage pressure (equinor#136) chore: fix typing of fluid composition docs: update docs and changelog for energy models (equinor#133) chore!: energy model type not allowed to change over time (equinor#131) docs: fix generic compressor example refactor: move NeqSimfluid creation into NeqSim wrapper chore: add 8.3 changelog chore: add fluid mixing checks ci: update pre-commit settings refactor: imrpove naming and documentation chore: add fluid mixing checks refactor: molar_mass_kg_per_mol is not used in the code fix: wrong standard_conditions_density when mixing two fluids test: add test for fluid stream mixing chore: merge queque fix: parse spaces as thousand separators from excel (equinor#107) refactor: NeqSim mapping (equinor#120) ...
… interstage pressure