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

fix!: include direct emitter results in ltp export #305

Merged
merged 7 commits into from
Dec 11, 2023

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented Nov 29, 2023

BREAKING CHANGE: change name from DIRECT_EMITTER to VENTING_EMITTER in input Yaml-file.

Why is this pull request needed?

Venting and fugitive emissions are currently modelled as FUELCONSUMERS (in most models), but it should be possible to get the same results if setting up these emission types as VENTING_EMITTERS (which is more correct).

  • Venting emitter results are not reported to ltp export
  • Loading/storage volumes are not reported to ltp export, but is required in Centuries. Currently not possible in eCalc, requires changes to the VENTING_EMITTERS, and more input from user in yaml
  • Change name from DIRECT_EMITTERS to VENTING_EMITTERS in input Yaml-file. (refactor: change name from direct to venting emitter #303)

What does this pull request change?

  • EmissionQuery is updated to include venting emitters. Up til now it has only included fuel consumers.

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-409?atlOrigin=eyJpIjoiZDkzMTFmNzg1ZTQ0NDI0M2EwYzhiY2RlZTNlN2E5YzgiLCJwIjoiaiJ9

@frodehk frodehk added the bug Something isn't working label Nov 29, 2023
@frodehk frodehk self-assigned this Nov 29, 2023
@frodehk frodehk requested a review from a team as a code owner November 29, 2023 13:45
Copy link
Collaborator

@TeeeJay TeeeJay left a comment

Choose a reason for hiding this comment

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

Nice catch, but I think we need some additional things:

  • add an emitter equivalent to what previously has been a fuel consumer with no fuel, to verify that the result is the same
  • add the emitter as a temporal model, to verify correctness with temporal models
  • verify that the snapshot does not change?

@TeeeJay
Copy link
Collaborator

TeeeJay commented Nov 30, 2023

comment from the initial change from direct_emitter to venting_emitter:

this is a "huge" breaking change, and should have been marked in the git commit message with refactor!: and BREAKING CHANGE in the footer. Changelog and migration guide must also be updated.

please include that as a part of this commit/PR then :)

@frodehk
Copy link
Contributor Author

frodehk commented Nov 30, 2023

comment from the initial change from direct_emitter to venting_emitter:

this is a "huge" breaking change, and should have been marked in the git commit message with refactor!: and BREAKING CHANGE in the footer. Changelog and migration guide must also be updated.

please include that as a part of this commit/PR then :)

You mean the breaking change, or only changelog/migration guide?

@TeeeJay
Copy link
Collaborator

TeeeJay commented Nov 30, 2023

comment from the initial change from direct_emitter to venting_emitter:

this is a "huge" breaking change, and should have been marked in the git commit message with refactor!: and BREAKING CHANGE in the footer. Changelog and migration guide must also be updated.

please include that as a part of this commit/PR then :)

You mean the breaking change, or only changelog/migration guide?

both :) ...

@frodehk frodehk requested a review from a team as a code owner November 30, 2023 10:01
@frodehk frodehk changed the title fix: include direct emitter results in ltp export fix!: include direct emitter results in ltp export Nov 30, 2023
@frodehk
Copy link
Contributor Author

frodehk commented Nov 30, 2023

Nice catch, but I think we need some additional things:

  • add an emitter equivalent to what previously has been a fuel consumer with no fuel, to verify that the result is the same
  • add the emitter as a temporal model, to verify correctness with temporal models
  • verify that the snapshot does not change?

I am not sure what we achieve with doing this. When modelling venting emitters as fuel consumers, it has to be energy usage to have emissions (i.e. a fuel rate). This is not correct, as venting emitters are emissions to air (not combustion). Hence, there will be a difference if we try to mimic the venting emissions as a fuel consumer using the DIRECT energy usage model. If we use no fuel the energy usage will be the same in the two approaches, but the fuel consumer will have no emissions. If we adjust the fuel and emission factors (for the fuel consumer approach), the emissions will be the same but the energy usage will be different (too high for the fuel consumer approach).

.for_period(period)
.to_volumes()
)
unit_in = emission_volumes.unit
Copy link
Contributor

Choose a reason for hiding this comment

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

We are making assumptions on unit here, right? Assuming all emissions have the same unit. So setting unit_in here is to make sure it is set if the above condition (for fuel_consumers) is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

emission.rate.unit is set before the query (e.g. t/d), in the to_volumes() it is converted to t. So the assumption is made earlier. The docs states that the emission rate should be in kg/day, and the assumption is done in FuelModel.evaluate_emissions(), where kg/d is converted to t/d - hence, assuming input is kg/d.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me, for this to fail the unit for fuel_consumers (emissions) and direct emitter would have to be different, and that is something we control.

It won't be an issue when adding units to venting_emitter either as we will convert into kg (probably). Is that something you are planning to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am adding unit to venting_emitter in another ongoing pr (will convert to kg as well).

Comment on lines 1 to 4
---
title: v8.3 to v8.4
description: v8.3 to v8.4 migration
sidebar_position: 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong title and description

@frodehk frodehk force-pushed the ECALC-409-include-direct-emission-results-in-ltp branch from f2ae513 to 6124f17 Compare December 11, 2023 12:02
@frodehk
Copy link
Contributor Author

frodehk commented Dec 11, 2023

Nice catch, but I think we need some additional things:

  • add an emitter equivalent to what previously has been a fuel consumer with no fuel, to verify that the result is the same
  • add the emitter as a temporal model, to verify correctness with temporal models
  • verify that the snapshot does not change?

I agree in the need of a test for venting emitters. I will add one as soon as the new pr with yaml classes is merged.

@frodehk frodehk merged commit f6b6371 into main Dec 11, 2023
6 checks passed
@frodehk frodehk deleted the ECALC-409-include-direct-emission-results-in-ltp branch December 11, 2023 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants