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.

Sign up for GitHub

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

Jump to bottom

#663 Fix crash when calling ecl_grid_reset_actnum #664

Merged
merged 4 commits into from
Oct 24, 2019
Merged

#663 Fix crash when calling ecl_grid_reset_actnum #664

merged 4 commits into from
Oct 24, 2019

Conversation

lindkvis
Copy link
Contributor

@lindkvis lindkvis commented Sep 25, 2019 •
edited
Loading

Issue
Resolves #663

Approach

  • Following review: reset active_index across the grid for the involved path in ecl_grid_set_active_index.
  • It does not appear necessary to do this for the fast path because that sets active_index even if it wasn't -1 before hand.

@ertomatic
Copy link
Collaborator

Can one of the admins verify this patch?

@lindkvis lindkvis removed their assignment Sep 29, 2019
@lindkvis
Copy link
Contributor Author

lindkvis commented Sep 29, 2019 •
edited
Loading

I was assigned to this, but I am the submitter. If there is any need to modify the patch, please let me know, but AFAIK this hasn't been evaluated yet. Unassigned myself.

@markusdregi
Copy link
Contributor

Thank you for your patch @lindkvis 👍 We will need some time to go through the consequences of the change. We will get back to you as soon as possible.

@markusdregi
Copy link
Contributor

@lindkvis Sorry for the late reply!

We have looked at the relevant code now and we think there might be an issue with your patch. In particular, ecl_coarse_cell_update_index is using active_index as an indicator for whether this course cell has already been updated. We suggest that instead of your edit, you reset the active index across the entire grid to -1 at the start of ecl_grid_set_active_index.

Also, could you please provide a test that fails on the current implementation that verifies the new patch?

@markusdregi
Copy link
Contributor

Dear @lindkvis. We are preparing a new release of libecl now. Are you interested in pushing this issue so that we get it in before that? I know that you are not necessarily following the releases of libecl, but I think potential bug-fixes etc would be easier to handle if we sync up a bit...

@lindkvis
Copy link
Contributor Author

@markusdregi . I have updated the pull request following review. Because we have to alter the coarse cell indices as well, which is in a private scope inaccessible from ecl_grid that led to a new method in ecl_coarse_cell. Please advice if this isn't what you meant.

As for data, we are not at liberty to share this and have no sharable data. I can at least ask @hhgs if he is able to make it available to you for testing.

andreabrambilla
andreabrambilla suggested changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@andreabrambilla andreabrambilla left a comment

Choose a reason for hiding this comment

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

The proposed solution seems correct now but I think there may be some unnecessary lines of code, I have added a few comments, please have a look at them, this PR is almost ready for merge

And thanks for putting efforts into this ;)

lib/ecl/ecl_grid.cpp Outdated Show resolved Hide resolved
lib/ecl/ecl_coarse_cell.cpp Outdated Show resolved Hide resolved
lib/ecl/ecl_grid.cpp Outdated
}
} else {
ecl_coarse_cell_type * coarse_cell = ecl_grid_iget_coarse_group(ecl_grid, cell->coarse_group);
ecl_coarse_cell_reset_active_index(coarse_cell, cell->active);
Copy link
Contributor

Choose a reason for hiding this comment

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

If my comment in ecl_coarse_grid.cpp is correct, then we do not need to loop through all the cells in order to retrieve the coarse cells. We can directly access the list of coarse cells in the grid and reset them all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the pull request. I don't really have any more time to put into this so I hope this is acceptable.

andreabrambilla
andreabrambilla approved these changes Oct 24, 2019
View reviewed changes
Copy link
Contributor

@andreabrambilla andreabrambilla 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, thanks for all your efforts 👍

Too bad we don't have a test for this, if you'll ever find a dataset with coarse cells that can be made freely available, please let us know

@andreabrambilla
Copy link
Contributor

jenkins test this please

@andreabrambilla andreabrambilla merged commit be33a98 into equinor:master Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreabrambilla andreabrambilla andreabrambilla approved these changes

@jokva jokva Awaiting requested review from jokva

@lars-petter-hauge lars-petter-hauge Awaiting requested review from lars-petter-hauge

@markusdregi markusdregi Awaiting requested review from markusdregi

@pgdr pgdr Awaiting requested review from pgdr

@xjules xjules Awaiting requested review from xjules

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Crash when calling ecl_grid_reset_actnum()
4 participants
@lindkvis @ertomatic @markusdregi @andreabrambilla