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

Separate Regularity from Core #246

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Conversation

TeeeJay
Copy link
Collaborator

@TeeeJay TeeeJay commented Oct 18, 2023

Why is this pull request needed?

This pull request is needed because of....

What does this pull request change?

Write summary of what this pull request changes if needed.

Issues related to this change:

@TeeeJay TeeeJay changed the title chore: one commit Separate Regularity from Core Oct 18, 2023
@TeeeJay TeeeJay force-pushed the ECALC-196-separate-regularity branch 2 times, most recently from 8057d84 to 8b226c9 Compare October 19, 2023 13:03
Copy link
Contributor

@jsolaas jsolaas left a comment

Choose a reason for hiding this comment

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

Looks good! Most changes seems expected to me, but I'm not sure why the PartialEmissionResult was needed. Is it because regularity/calendar day was needed to calculate intensities?

for key in emissions
}

def to_full_result(
Copy link
Contributor

Choose a reason for hiding this comment

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

to_full_emission_result?

@@ -903,3 +1253,35 @@ def get_results(self) -> EnergyCalculatorResult:
emission_results=self.emission_results,
variables_map=self.variables_map,
)

def convert_to_timeseries(
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the name here?

timesteps=variables_map.time_vector,
values=list(
rate.evaluate(
variables_map.get_subset_from_period(period).variables,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably outdated?

@jsolaas jsolaas force-pushed the ECALC-196-separate-regularity branch from 8b226c9 to 264edeb Compare October 19, 2023 13:52
@jsolaas jsolaas marked this pull request as ready for review October 19, 2023 13:56
@jsolaas jsolaas requested a review from a team as a code owner October 19, 2023 13:56
Regularity should not be transferred or handled in
core. At the time data arrives in core, it should only
be stream day, and we should not have to relate to "what
type of rate" is it, until we decide whether the user
output should be calendar or stream day (default) based
on user needs.

Refs ECALC-196
@jsolaas jsolaas force-pushed the ECALC-196-separate-regularity branch from 264edeb to 206e69e Compare October 19, 2023 13:56
@jsolaas jsolaas merged commit 714888b into main Oct 20, 2023
6 checks passed
@jsolaas jsolaas deleted the ECALC-196-separate-regularity branch October 20, 2023 06:43
frodehk pushed a commit that referenced this pull request Oct 24, 2023
Regularity should not be transferred or handled in
core. At the time data arrives in core, it should only
be stream day, and we should not have to relate to "what
type of rate" is it, until we decide whether the user
output should be calendar or stream day (default) based
on user needs.

Refs ECALC-196
equinor-schen pushed a commit that referenced this pull request Nov 22, 2023
Regularity should not be transferred or handled in
core. At the time data arrives in core, it should only
be stream day, and we should not have to relate to "what
type of rate" is it, until we decide whether the user
output should be calendar or stream day (default) based
on user needs.

Refs ECALC-196
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants