-
Notifications
You must be signed in to change notification settings - Fork 59
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
New simulation time series plugin #867
New simulation time series plugin #867
Conversation
…v_file() + misc refactoring
…rt of per-real arrow files
…mulation-time-series-new-plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @jorgenherje 👏🚀 Looks nice!
I like that you also apparently has laid the ground for reusing parts of the functions in other plugins as well going forward.
There are some TODO
s in the code (I've only commented on a couple of them). Should we stick with them in code only, or should we make issue of them as well as reminders?
Besides that I have only a couple of minor comments, then this is ready to merge IMO when we see green light towards example data.
webviz_subsurface/plugins/_simulation_time_series/types/derived_ensemble_vectors_accessor.py
Outdated
Show resolved
Hide resolved
...z_subsurface/plugins/_simulation_time_series/types/derived_ensemble_vectors_accessor_impl.py
Outdated
Show resolved
Hide resolved
- Removing incorrect comments (Mostly TODO's) - Have made GitHub-issues based on some of the comments
Rename from DerivedEnsembleVectorsAccessor to DerivedVectorsAccessor
… on invalid frequency. - Due to user request, the verification is removed, and user must be aware tha vector metadata incosistency can occur. Note retrieveing of metadata gives first valid metadata, thus invalid metadata should not affect the plugin. - Add ValueError when converting input sampling str to Frequency results in `None` i.e. raw data. - Fix incorrect comment
…mulation-time-series-new-plugin
…or#957) * 1. Removed unnecessary viewState management, using initialViewState to let deckGL manage the state 2. Set axes rotation along Y axis for 3D view Fix equinor#956 Fix equinor#867 * update snapshot * capture screenshots * capture screenshots * new screenshots updated Co-authored-by: Shadab Khan <[email protected]> Co-authored-by: Sanghavi <[email protected]>
Create new simulation time series plugin
SimulationTimeSeries
Based on the functionality in original
ReservoirSimulationTimeSeries
plugin, with additional functionality requests listed in following issue: #698.This PR includes and closes parts of the listed requests:
Review feedback
Please give feedback on following points:
_property_serialization
-foldertypes
-folderutils
-folder be handy for others and placed inwebviz-subsurface/_utils
-folder?webviz_subsurface/_utils
-folder. Could be reusable, and additional functionality can be added based on needs.Summary
The work includes structuring and refactoring of code for enhanced plugin implementation. Goal is to have code separated on functionality (callbacks, layout, business logic etc.), convert dash property to strongly typed arguments for "business logic". Clarifies intended usage and testability, preventing loosely typed arguments resulting in passing incorrect data.
Utilize best practice example in
webviz-plugin-boilerplate
.Main points
webviz-plugin-boilerplate
Delta Ensemble
:Delta Ensemble
Delta Ensemble
as selectable ensemblevectors
- subplots per selected vectorensemble
- subplots per selected ensembleTodo