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: wrong handling of values and timesteps in temporal models #261

Conversation

frodehk
Copy link
Contributor

@frodehk frodehk commented Oct 30, 2023

Why is this pull request needed?

Problem when extending time series for compressors across temporal models in EnergyModelBaseResult.extend(). This is basically related to how None-values are treated. Two bugs:

  • Loss of data: When first temporal model contains None and next temporal model contains real values, the merge/extend function evaluates to None - ignoring data from second temporal model.
  • Mismatch between timesteps and values: When first temporal model contains real values, and next temporal model contains None, the merge/extend function evaluates to the values of the first temporal model. The timesteps in the second model are ignored, hence a mismatch between number of timesteps and number of values are introduced for the actual time series.

What does this pull request change?

  • CompressorTrainResultSingleStep: Ensure that all time series have values or NaN for all valid timesteps, even if input is None
  • CompressorModelSampled: Ensure that all time series have values or NaN for all valid timesteps, even if input is None
  • Update snapshots
  • Add test

Issues related to this change:

https://equinor-ecalc.atlassian.net/browse/ECALC-376?atlOrigin=eyJpIjoiNzY4MzVlMWU2ZjEzNDMwNzhiYzNhMDZkMmExYzljOTQiLCJwIjoiaiJ9

@frodehk frodehk self-assigned this Oct 30, 2023
@frodehk frodehk requested a review from a team as a code owner October 30, 2023 14:26
@frodehk frodehk added the bug Something isn't working label Oct 30, 2023
@frodehk frodehk force-pushed the ECALC-376-wrong-handling-of-values-and-timesteps-in-temporal-models branch from d4cd52d to ce8dc82 Compare October 31, 2023 15:03
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.

it looks good, but all snapshots are expanding null to NaN..I guess we have no cases where real values are actually overwritten? Good with test. Need UAT to have a look to QA this :) Good job though :)

@frodehk
Copy link
Contributor Author

frodehk commented Nov 1, 2023

it looks good, but all snapshots are expanding null to NaN..I guess we have no cases where real values are actually overwritten? Good with test. Need UAT to have a look to QA this :) Good job though :)

Yes, I think None is resolved to null in json (?). And, None is now filled with nans to ensure correct number of values vs timesteps.

@frodehk frodehk force-pushed the ECALC-376-wrong-handling-of-values-and-timesteps-in-temporal-models branch from 8f63a7d to 47f2fac Compare November 2, 2023 08:26
@frodehk frodehk force-pushed the ECALC-376-wrong-handling-of-values-and-timesteps-in-temporal-models branch from 47f2fac to 7b69ff0 Compare November 20, 2023 12:52
@frodehk frodehk merged commit 4e20264 into main Nov 21, 2023
6 checks passed
@frodehk frodehk deleted the ECALC-376-wrong-handling-of-values-and-timesteps-in-temporal-models branch November 21, 2023 09:25
equinor-schen pushed a commit that referenced this pull request Nov 22, 2023
* fix: wrong handling of values and timesteps in temporal models
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.

2 participants