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

Remove direct import from SimulationTimeSeries utilities #1161

Conversation

jorgenherje
Copy link
Collaborator

@jorgenherje jorgenherje commented Nov 11, 2022

Various plugins performs direct import of utility objects/functions from SimulationTimeSeries.

Move the respective utils to a common top-level utility folder. Utilities which are too general, and does not belong in the top-level utility folder, are moved into the specific plugins to prevent misusage.


Done:

  • Moved the ProviderSet to webviz_subsurface/_utils for common usage.
    • Renamed ProviderSet to EnsembleSummaryProviderSet as the utility is now moved out of scope of SimulationTimeSeries. There are multiple providers in webviz-subsurface and a common utility must clarify its scope - i.e. set for EnsembleSummaryProvider.
    • Note:
      • names()-method is renamed to provider_names() for clarity.
      • The provider set functionality is assuming same implementation of EnsembleSummaryProvider interface across all provides in set.
  • Moved the ProviderSet creator methods to webviz_subsurface/_utils
    • File renamed to ensemble_summary_provider_set_factory.py according to renaming to EnsembleSummaryProviderSet
  • Removed webviz_subsurface/_utils/datetime_utils.py
    • Was a direct copy of utility in SimulationTimeSeries-plugin folder. The implementations assumes data with daily frequency, thus the date conversion to_str and from_str methods can result in loss of data if date frequency is higher than daily. This should not be common utility methods. File deleted and utility methods added directly in respective plugins.
  • Updated of imports according to new file placement
  • Updated/moved unit tests to new folders according to new naming and placement

Closes: #1106

@jorgenherje jorgenherje added the enhancement 🚀 New feature or request label Nov 11, 2022
@jorgenherje jorgenherje self-assigned this Nov 11, 2022
@jorgenherje jorgenherje marked this pull request as draft November 11, 2022 14:30
@jorgenherje jorgenherje marked this pull request as ready for review November 14, 2022 10:41
@anders-kiaer anders-kiaer changed the title Remove direct import from SimulationTimeSeries utilities Remove direct import from SimulationTimeSeries utilities Nov 17, 2022
Copy link
Collaborator

@anders-kiaer anders-kiaer left a comment

Choose a reason for hiding this comment

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

Very good 👏 LGTM! 💯

@anders-kiaer anders-kiaer added the next release 🚢 To be included in next release label Nov 17, 2022
…ries

- Move definition of ProviderSet til webviz_subsurface/_utils
- Move provider set create methods and rename file
webviz_subsurface/_utils/provider_set_factory.py
- Remove webvis_subsurface/_utils/datetime_utils.py as it should not be
a common util. Due to limitation of date data frequency of daily it can be
misused and one can loose date content. It should be handled
specific in plugins.
- Update imports accordingly
- Update unit test file placement accordingly
- Rename as the `ProviderSet` is moved out of SimulationTimeSeries scope.
webviz-subsurface has multiple type of providers in `_providers`,
thereby the type of provider is specified in naming when the utility is
moved to top-level `utils` folder
- Update imports and file-naming accordingly
@jorgenherje jorgenherje force-pushed the EQ_1106-move-simulation-time-series-utilities branch from 65e1a1b to 5e69950 Compare November 18, 2022 07:48
@jorgenherje jorgenherje merged commit d26c915 into equinor:master Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🚀 New feature or request next release 🚢 To be included in next release
Projects
Status: Done 🏁
Development

Successfully merging this pull request may close these issues.

Remove direct import from SimulationTimeSeries into other plugins
2 participants