Skip to content

Report pinched cells#520

Merged
bska merged 2 commits intoOPM:masterfrom
joakim-hove:report-pinched-cells
Apr 16, 2021
Merged

Report pinched cells#520
bska merged 2 commits intoOPM:masterfrom
joakim-hove:report-pinched-cells

Conversation

@joakim-hove
Copy link
Member

Problem with: #519 - only reported cells which were also joined with a pinch. Current version reports nnc pinching and removed cells independently.

@joakim-hove
Copy link
Member Author

jenkins build this please

Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Small nitpicks

/// \param clip_z if true, the grid will be clipped so that the top and bottom will be planar.
/// \param poreVolume pore volumes for use in MINPV processing, if asked for in deck
///
/// The return value is a list of global indices to the cells which have
Copy link
Member

@blattms blattms Apr 13, 2021

Choose a reason for hiding this comment

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

Any reason not to use /// \return a list ...

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason - apart from ignorance; updated now.

///
/// The return value is a list of global indices to the cells which have
/// been removed in the grid processing due to small pore volume.
/// Observe that the processEclipseFormat() function only runs on rank
Copy link
Member

Choose a reason for hiding this comment

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

... function only returns cell indices on rank 0. vector empty on other ranks.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@joakim-hove joakim-hove force-pushed the report-pinched-cells branch from 9bc5d79 to 96797d3 Compare April 13, 2021 09:56
@blattms
Copy link
Member

blattms commented Apr 13, 2021

jenkins build this please

@bska
Copy link
Member

bska commented Apr 13, 2021

jenkins build this please

Just be aware that this is going to generate at least one regression failure. The current reference solutions are out of date with respect to the master branch of opm-simulators. I'm using OPM/opm-common#2424 to rectify that.

@bska
Copy link
Member

bska commented Apr 13, 2021

jenkins build this please

@joakim-hove
Copy link
Member Author

Can this be merged?

@bska
Copy link
Member

bska commented Apr 16, 2021

Can this be merged?

If it's all the same I'd prefer to get #517 merged first. I think it's unlikely that #517 will generate a merge conflict with this PR, but there's nevertheless a non-zero risk and this PR is smaller so conflict resolution should be easier here.

@joakim-hove
Copy link
Member Author

If it's all the same I'd prefer to get #517 merged first.

Well - no big deal; this PR is ready to be merged now - I don't know the readyness of #517?

@GitPaean
Copy link
Member

GitPaean commented Apr 16, 2021

I am okay this one goes in first. #517 might takes time to review. I will take care of it if something goes wrong by merging this.

@bska
Copy link
Member

bska commented Apr 16, 2021

I don't know the readiness of #517?

I was about to merge it, but then I noticed a change to the module requirements and now I think that it'll be at least until next week before that PR is ready. I'll merge this now.

@bska bska merged commit 9b5b62b into OPM:master Apr 16, 2021
@joakim-hove joakim-hove deleted the report-pinched-cells branch April 20, 2021 13:58
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