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

Sets up a correct map from element to interior element index. #137

Merged
merged 2 commits into from
Jan 18, 2017
Merged

Sets up a correct map from element to interior element index. #137

merged 2 commits into from
Jan 18, 2017

Conversation

blattms
Copy link
Member

@blattms blattms commented Jan 18, 2017

I assume that is what the code should do. Not 100% sure though.
It also closes #105 by actually resizes the arrays we store values using random access in.

@blattms
Makes CollectDataToIORank::localIndexMap_ have nonzero size when acce…
dde79da 
…ssing.

While it may not produce memory errors it is semantically wrong to insert
entries with random acces into a vector that has reserved enough memory to
hold the values but still has size 0.
@blattms
Make CollectDataIORank::localIndexMap_ as map from element to interio…
09db2fd 
…r element index.

Previously it was an identity map.
@blattms
Copy link
Member Author

blattms commented Jan 18, 2017

It turns out that my changes raise an assertion in a sequential run. Why is this code even executed in this case?

@blattms
Copy link
Member Author

blattms commented Jan 18, 2017 •
edited
Loading

It happens in collecttoiorank.hh:127. As with my commit dde79da localIndexMap_ has nonzero size now this check is actually triggered. Seems like issue #105 was a feature too. It disabled some parallel code in a sequential run.

@andlaus
Copy link
Contributor

andlaus commented Jan 18, 2017

thanks, will merge.

Why is this code even executed in this case?

I can't tell you for sure, but I guess it is easier to just create this object unconditionally.

@andlaus andlaus merged commit edd7095 into OPM:master Jan 18, 2017
@blattms
Copy link
Member Author

blattms commented Jan 18, 2017

I said

It turns out that my changes raise an assertion in a sequential run

Your merge might have been a bit premature...

@blattms blattms mentioned this pull request Jan 18, 2017
Merged
@andlaus
Copy link
Contributor

andlaus commented Jan 18, 2017

yeah, but I understood that the PR has been updated to fix it...

@andlaus
Copy link
Contributor

andlaus commented Jan 18, 2017

anyway, stand-alone ebos and flow_ebos seem to work with optimizations in sequential mode, so this is not a big deal IMO...

@blattms
Copy link
Member Author

blattms commented Jan 18, 2017

I would not be so sure that everybody uses -DNDEBUG when compiling OPM. I only set this for benchmarking.

@blattms blattms mentioned this pull request Jan 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PR #101 changed semantics and introduced memory bugs
2 participants
@blattms @andlaus