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

CO2Leakage: Visual improvements, split plots into tabs, add slider, add options for which category to use for color/markings #1269

Merged
merged 38 commits into from
Jun 24, 2024

Conversation

AudunSektnanNR
Copy link
Contributor

@AudunSektnanNR AudunSektnanNR commented Apr 15, 2024

Ready for review.


  • 📖 I have considered adding a new entry in CHANGELOG.md, and added it if should be communicated there.

@AudunSektnanNR AudunSektnanNR marked this pull request as ready for review April 16, 2024 13:43
@AudunSektnanNR
Copy link
Contributor Author

Some failing code style checks in different plugin, _map_viewer_fmu, not sure why. Tests in co2_leakage should be fine.

* Temp test 1.

* Temp test 2.

* Quick fix 1.

* Quick fix 2.

* CCS-127: Clean up fix.
@andreas-el
Copy link
Contributor

Some failing code style checks in different plugin, _map_viewer_fmu, not sure why. Tests in co2_leakage should be fine.

This will likely be resolved by #1274 and should not be affecting this PR.

@andreas-el
Copy link
Contributor

This PR could benefit from some cleanup and rebasing prior to reviewing. Most commits with Fix pylint|black|mypy. Merge pull|branch etc should not be committed to master/main regardless.
Do you intend to keep some of the commits separate from others, or were you planning to squash the whole PR once approved?

@AudunSektnanNR
Copy link
Contributor Author

This PR could benefit from some cleanup and rebasing prior to reviewing. Most commits with Fix pylint|black|mypy. Merge pull|branch etc should not be committed to master/main regardless. Do you intend to keep some of the commits separate from others, or were you planning to squash the whole PR once approved?

The plan was to squash all the commits when merging into the master branch. This is what we have done in earlier pull requests, even though the pull request might consist of changes that are not always related to each other (other than being in CO2Leakage-plugin). Should we change this approach? I guess the alternative is to make separate issues in this repo and make a pull request for each issue separately (or combine a few issues in one PR, but with 1 commit per issue).

@andreas-el
Copy link
Contributor

This PR could benefit from some cleanup and rebasing prior to reviewing. Most commits with Fix pylint|black|mypy. Merge pull|branch etc should not be committed to master/main regardless. Do you intend to keep some of the commits separate from others, or were you planning to squash the whole PR once approved?

The plan was to squash all the commits when merging into the master branch. This is what we have done in earlier pull requests, even though the pull request might consist of changes that are not always related to each other (other than being in CO2Leakage-plugin). Should we change this approach? I guess the alternative is to make separate issues in this repo and make a pull request for each issue separately (or combine a few issues in one PR, but with 1 commit per issue).

First off, I'm not deciding anything here, only voicing a single opinion.
I think as a general rule that you should rebase/squash/merge some of the commits to make reviewing easier.
When you do these merge-commits inline, this becomes much more difficult.

@HansKallekleiv HansKallekleiv self-requested a review June 24, 2024 09:46
@HansKallekleiv HansKallekleiv merged commit 784e462 into equinor:master Jun 24, 2024
6 checks passed
@AudunSektnanNR AudunSektnanNR deleted the develop branch June 24, 2024 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants