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

CO₂ migration plugin #994

Merged
merged 101 commits into from
Nov 9, 2022
Merged

Conversation

vegardkv
Copy link
Contributor

@vegardkv vegardkv commented Mar 29, 2022

Aimed at #993

Implements a new plugin for CO2 leakage analysis, using the new layout framework.

Example data is available via equinor/webviz-subsurface-testdata#136

(Known) remaining tasks:

  • Code formatting

@anders-kiaer anders-kiaer changed the title Co2 migration plugin CO₂ migration plugin Mar 29, 2022
@vegardkv
Copy link
Contributor Author

All comments should now have been addressed. There are some discussion points remaining, but I think they mostly relate to"big-picture" issues.

@vegardkv
Copy link
Contributor Author

Fixed black and pylint reports. Came across an issue related to dependencies. This plugin implements a contour line generator which depends on matplotlib and shapely. I cannot see that these are used elsewhere. What is the policy for new dependencies?

There are some options here:

  • Allow them (i.e. do nothing)
  • Enable functionality based on package availability. If an ImportError occurs, this specific feature (contour lines) is disabled.
  • Remove the functionality (for now). The implementation is experimental and not the most important. Its goal is to generate contour lines, but this can be left to the deck.gl component instead. It seems to have contour features, as well as Contour layer webviz-subsurface-components#622. However, I couldn't find a way to do make it work as I wanted.

@vegardkv vegardkv marked this pull request as ready for review September 15, 2022 09:23
@HansKallekleiv
Copy link
Collaborator

Fixed black and pylint reports. Came across an issue related to dependencies. This plugin implements a contour line generator which depends on matplotlib and shapely. I cannot see that these are used elsewhere. What is the policy for new dependencies?

There are some options here:

  • Allow them (i.e. do nothing)
  • Enable functionality based on package availability. If an ImportError occurs, this specific feature (contour lines) is disabled.
  • Remove the functionality (for now). The implementation is experimental and not the most important. Its goal is to generate contour lines, but this can be left to the deck.gl component instead. It seems to have contour features, as well as Contour layer webviz-subsurface-components#622. However, I couldn't find a way to do make it work as I wanted.

For Equinor usage (komodo) I don't see this as a problem as both packages are already included.
@anders-kiaer , @asnyv any thoughts?

@asnyv
Copy link
Collaborator

asnyv commented Sep 23, 2022

Sorry, didn't see this until now.

We have tried before to avoid including unnecessary dependencies, and intentionally removed matplotlib as a dependency before due to size vs added value (see: #99). Of course not saying that we cannot add more dependencies, but think in a growing repo with several contributors, we should be a bit conservative. If the functionality is more or less experimental, at least I suggest you go for option 2 or 3 from your suggestions (so either try to import and disable if not available, or drop it). If it is found to be necessary later we can rather include the dependencies then.

@vegardkv
Copy link
Contributor Author

Added a check for matplotlib + shapely availability. If an ImportError is raised, the functionality is effectively turned off. I think this is a reasonable solution for now, as this is not a critical part of the plugin. It is an experimental feature that we might want to improve upon later.

That being said, I suspect matplotlib and shapely are implicit dependencies of webviz_subsurface after all, probably via xtgeo? (I didn't really test properly until after the commit above). When I try to run this plugin, I get ImportError if matplotlib or shapely is not installed, originating from other parts of the code.

Since matplotlib/shapely is not explicit dependencies, I think the solution implemented in b7764aa should be fine

Copy link
Collaborator

@HansKallekleiv HansKallekleiv 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 to me 👍

@HansKallekleiv HansKallekleiv added the next release 🚢 To be included in next release label Oct 24, 2022
@HansKallekleiv HansKallekleiv merged commit 11cfb33 into equinor:master Nov 9, 2022
@vegardkv vegardkv deleted the co2-migration-plugin branch November 10, 2022 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next release 🚢 To be included in next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants