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

Adds resampling of observations #272

Merged

Conversation

edubarrosTNO
Copy link
Contributor

@edubarrosTNO edubarrosTNO commented Nov 30, 2020

Adds resampling of observation dates at requested frequency by finding the nearest date among existing observation dates.


Contributor checklist

  • 🎉 This PR partially addresses Too many observations used when history matching Norne #206.
  • 📜 I have broken down my PR into the following tasks:
    • Resampling observation dates based on requested frequency.
    • Adds (optional) resampling entry to the config parser.
  • 🤖 I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR.
  • 📖 I have considered adding a new entry in CHANGELOG.md.
  • 📚 I have considered updating the documentation.

@edubarrosTNO edubarrosTNO added the enhancement New feature or request label Nov 30, 2020
@wouterjdb wouterjdb marked this pull request as draft November 30, 2020 16:04
@edubarrosTNO edubarrosTNO marked this pull request as ready for review November 30, 2020 18:19
@edubarrosTNO edubarrosTNO self-assigned this Nov 30, 2020
Copy link
Collaborator

@wouterjdb wouterjdb left a comment

Choose a reason for hiding this comment

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

LGTM. Two minor comments:

  1. See review, I suggest to indicate that this new function is an internal function by using an underscore. This also makes it clearer for the reader it is not really a test as such.
  2. You now always test with resampling. Could you maybe run, in that same test, twice the same function. Once with resampling (and check that the output is as expect) and once without resampling (and check that the output is as expected).

Copy link
Collaborator

@wouterjdb wouterjdb left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@wouterjdb wouterjdb merged commit d81a383 into equinor:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants