search

OPM / opm-output

This repository is intended for output-writer functionality for the flow simulators in OPM
http://www.opm-project.org
GNU General Public License v3.0
3 stars 21 forks source link

Extra data in restart files. #172

Closed joakim-hove closed 7 years ago

joakim-hove commented 7 years ago

Will require some impedance matching in opm-simulators; will @atgeirr take that part?

atgeirr commented 7 years ago

will @atgeirr take that part?

I will.

joakim-hove commented 7 years ago

Hmmmm - it works on my machine :angry:

joakim-hove commented 7 years ago

jenkins build this please

joakim-hove commented 7 years ago

@akva2: What was the trick to use valgrind as test command?

akva2 commented 7 years ago

ctest -T memcheck

joakim-hove commented 7 years ago

ctest -T memcheck

Thank you.

atgeirr commented 7 years ago

The valgrind test fails for me (do not have valgrind installed).

joakim-hove commented 7 years ago

The (hardcoded!) valgrind stuff is purely WIP; I am just trying to understand why the test fails on Travis.

atgeirr commented 7 years ago

The jenkins run from opm-simulators was successful, are there still things you want to do here? (Other than possibly squashing or cleanup.)

joakim-hove commented 7 years ago

I am uncomfortable merging before the Travis failure is understood/resolved.

atgeirr commented 7 years ago

I am uncomfortable merging before the Travis failure is understood/resolved.

I get that. I'll see if I can provoke a failure on my system.

joakim-hove commented 7 years ago

Thank you for the suggestions and comments - I will try to look at this afternoon.

  1. mars 2017 09:03 skrev "Atgeirr Flø Rasmussen" notifications@github.com:

@atgeirr commented on this pull request.

In opm/output/eclipse/RestartIO.cpp https://github.com/OPM/opm-output/pull/172#discussion_r103868639:

@@ -509,8 +541,10 @@ void save(const std::string& filename, data::Wells wells, const EclipseState& es, const EclipseGrid& grid,

  • std::map<std::string, std::vector> extra_data,

I looked at this and found no errors. There are a few minor things I would change, though, that I did not notice before. The wells and extra_data arguments should be passed as const references (unlike cells they are not modified inside this function). Also, I'd put the reserved_keys inside the checking scope below. Not that it should matter, but could there be a problem with multithreaded calls to this function, because of the nontrivial static variable (perhaps someone with more knowledge about this can enlighten me)? I think removing the static optimization would not harm performance in any case since this is after all a function that writes to disk.

I cannot reproduce, but on Travis the error is triggered in the function that uses the default parameter. Perhaps removing that default parameter could shed some light on it?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/OPM/opm-output/pull/172#pullrequestreview-24671130, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQqwWUWivQKX5BWA98Pxtbs69FktFuIks5rhnfkgaJpZM4MM7jj .

atgeirr commented 7 years ago

I see that Travis tested ok (in opm-output) now. Do you know what caused the previous problems?

joakim-hove commented 7 years ago

Do you know what caused the previous problems?

To be honest no - but I think the failure happened in a code path mixing C++ exceptions and C code; should in my opinion only have resulted in a resource leak and not a full blown error, but anyway rewritten now,

atgeirr commented 7 years ago

code path mixing C++ exceptions and C code

That sounds like it could easily blow up... I'm curious about where we have such a mix though?

joakim-hove commented 7 years ago

I'm curious about where we have such a mix though?

It was in the RestartIO::save( ) function which first opened a ert handle to an output file, and then could throw a C++ exception when the size of the cells data was wrong. Anyway - fixed now.