Let Entity return geometry as a copy. by blattms · Pull Request #334 · OPM/opm-grid

blattms · 2018-06-19T10:37:41Z

Geometry is lightweight enough and this is what DUNE's grid interface mandates.
Performance for norne stays the same.
Closes PR #328

blattms · 2018-06-19T10:40:27Z

jenkins run norne please

atgeirr · 2018-06-25T09:44:12Z

I don't get why this should change any bit of the solution, will rerun Jenkins to be certain it was not a temporary fluke in the midst of a data update or something.

atgeirr · 2018-06-25T09:44:19Z

jenkins build this please

atgeirr · 2018-06-28T12:37:50Z

To be clear, I want to eventually merge this, but the failing checks makes me nervous and I want it to be investigated before merging.

atgeirr · 2018-08-01T11:52:37Z

jenkins build this please

blattms · 2018-08-01T12:29:07Z

The problem should still be there. And I have no idea what the problem really is.

atgeirr · 2018-08-01T13:40:49Z

The problem should still be there. And I have no idea what the problem really is.

I retested because I could not reproduce locally. Do you get the test failures on your Linux system?

blattms · 2018-08-02T09:12:11Z

Let me check again.

blattms · 2018-08-02T09:49:59Z

Yes I do get

/home/mblatt/src/dune/opm-2.6/opm-upscaling/tests/common/test_gravitypressure.cpp(317): error: in "/BaseCase": difference{0.111111} between c[i]{-999.99999999999977} and e[i]{-899.99999999999977} exceeds 0.0001%
/home/mblatt/src/dune/opm-2.6/opm-upscaling/tests/common/test_gravitypressure.cpp(317): error: in "/BaseCase": difference{0.428571} between c[i]{-999.99999999999977} and e[i]{-699.99999999999977} exceeds 0.0001%
...

when running test_gravitypressure. But I am unsure whether this is the same as the error that we got last time.

blattms · 2018-08-02T10:09:45Z

I have an upcoming fix for upscaling.

blattms · 2018-08-02T10:19:56Z

jenkind build this with opm-upscaling=261 please

blattms · 2018-08-02T10:24:13Z

jenkins build this opm-upscaling=261 please

blattms · 2018-08-02T10:27:46Z

Beware: PR OPM/opm-upscaling#261 should be merged before this one.

atgeirr · 2018-08-02T11:24:31Z

Thanks for fixing the first test error, it really did expose a bug in the old code. The second error (ignoring the TCPU thing that will be fixed soonish) is still not clear to me. Can you reproduce it? Do you get identical results apart from timing? The solution is not changing much, it is the fact that it changes at all that has me worried.

blattms · 2018-08-03T08:08:43Z

Not sure what you mean

atgeirr · 2018-08-03T08:32:38Z

Not sure what you mean

I think you have to be more specific, which sentence(s) is(are) unclear?

blattms · 2018-08-03T12:49:40Z

Sorry for not being clear. I wondered what the second failure is that you are seeing. I only saw the TCPU stuff.

atgeirr · 2018-08-03T12:58:38Z

There are two failures remaining, and only one is the TCPU one. In Jenkins only one is visible until you click the Test Result link, it's confusing. Direct link to failed test:

https://ci.opm-project.org/job/opm-grid-PR-builder/71/testReport/(root)/mpi/compareECLFiles_flow_SPE1CASE2_THERMAL/

blattms · 2018-08-03T17:41:34Z

Seems like I messed up this one, too. Was magically closed and has no commits now.

blattms · 2018-08-03T17:49:10Z

jenkins build this please

blattms · 2018-08-09T11:31:38Z

Unfortunately, I am having troubles reproducing the memory errors in test_transmissibilitymultipliers

akva2 · 2018-08-09T11:37:18Z

/home/akva/kode/opm/super-build/opm-grid/opm/grid/cpgrid/../CpGrid.hpp: In member function ‘const Vector& Dune::CpGrid::cellCentroid(int) const’:
/home/akva/kode/opm/super-build/opm-grid/opm/grid/cpgrid/../CpGrid.hpp:969:97: warning: returning reference to temporary [-Wreturn-local-addr]
             return current_view_data_->geomVector<0>()[cpgrid::EntityRep<0>(cell, true)].center();

akva2 · 2018-08-09T11:45:58Z

==20424==    at 0x8A54EE: double Opm::UgGridHelpers::getCoordinate<Opm::UgGridHelpers::CpGridCentroidIterator<&(Dune::CpGrid::cellCentroid(int) const)> >(Opm::UgGridHelpers::CpGridCentroidIterator<&(Dune::CpGrid::cellCentroid(int) const)>, int) (GridHelpers.hpp:395)
==20424==    by 0x8A0B95: void tpfa_htrans_compute<Dune::CpGrid>(Dune::CpGrid const*, double const*, double*) (TransTpfa_impl.hpp:97)
==20424==    by 0x89B38C: void Opm::DerivedGeology::update<Opm::BlackoilPropsAdFromDeck, Dune::CpGrid>(Dune::CpGrid const&, Opm::BlackoilPropsAdFromDeck const&, Opm::EclipseState const&, double const*) (GeoProps.hpp:125)
==20424==    by 0x896871: Opm::DerivedGeology::DerivedGeology<Opm::BlackoilPropsAdFromDeck, Dune::CpGrid>(Dune::CpGrid const&, Opm::BlackoilPropsAdFromDeck const&, Opm::EclipseState const&, bool, double const*) (GeoProps.hpp:79)
==20424==    by 0x88AD48: TransmissibilityMultipliersCpGrid::test_method() (test_transmissibilitymultipliers.cpp:282)
==20424==    by 0x88AB3A: TransmissibilityMultipliersCpGrid_invoker() (test_transmissibilitymultipliers.cpp:257)
==20424==    by 0x8B6987: boost::unit_test::ut_detail::unused boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()>(void (*&)()) (callback.hpp:56)
==20424==    by 0x8B679C: boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke() (callback.hpp:89)
==20424==    by 0x6215CB0: ??? (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==20424==    by 0x61F5995: boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==20424==    by 0x61F61B2: boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)
==20424==    by 0x6215DE1: boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) (in /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0)

akva2 · 2018-08-09T13:00:36Z

Thread 1 "test_transmissi" received signal SIGSEGV, Segmentation fault.
0x00000000008a54ee in Opm::UgGridHelpers::getCoordinate<Opm::UgGridHelpers::CpGridCentroidIterator<&(Dune::CpGrid::cellCentroid(int) const)> > (t=..., i=0)
    at /home/akva/kode/opm/super-build/opm-grid/opm/grid/GridHelpers.hpp:395
395         return (*t)[i];
(gdb) where
#0  0x00000000008a54ee in Opm::UgGridHelpers::getCoordinate<Opm::UgGridHelpers::CpGridCentroidIterator<&(Dune::CpGrid::cellCentroid(int) const)> > (t=..., i=0)
    at /home/akva/kode/opm/super-build/opm-grid/opm/grid/GridHelpers.hpp:395
#1  0x00000000008a0b96 in tpfa_htrans_compute<Dune::CpGrid> (G=0x153fc20, 
    perm=0x154f070, htrans=0x154f670)
    at /home/akva/kode/opm/super-build/opm-grid/opm/grid/transmissibility/TransTpfa_impl.hpp:97
#2  0x000000000089b38d in Opm::DerivedGeology::update<Opm::BlackoilPropsAdFromDeck, Dune::CpGrid> (this=0x7fffffffab00, grid=..., props=..., eclState=..., grav=0x0)
    at /home/akva/kode/opm/super-build/opm-simulators/opm/autodiff/GeoProps.hpp:125
#3  0x0000000000896872 in Opm::DerivedGeology::DerivedGeology<Opm::BlackoilPropsAdFromDeck, Dune::CpGrid> (this=0x7fffffffab00, grid=..., props=..., eclState=..., 
    use_local_perm=false, grav=0x0)
    at /home/akva/kode/opm/super-build/opm-simulators/opm/autodiff/GeoProps.hpp:79
#4  0x000000000088ad49 in TransmissibilityMultipliersCpGrid::test_method (
    this=0x7fffffffd4b7)
    at /home/akva/kode/opm/super-build/opm-simulators/tests/test_transmissibilitymultipliers.cpp:282
#5  0x000000000088ab3b in TransmissibilityMultipliersCpGrid_invoker ()
    at /home/akva/kode/opm/super-build/opm-simulators/tests/test_transmissibilitymultipliers.cpp:257
#6  0x00000000008b6988 in boost::unit_test::ut_detail::invoker<boost::unit_test::ut_detail::unused>::invoke<void (*)()> (this=0x7fffffffd517, 
    f=@0x10ef518: 0x88ab18 <TransmissibilityMultipliersCpGrid_invoker()>)
    at /usr/include/boost/test/utils/callback.hpp:56
#7  0x00000000008b679d in boost::unit_test::ut_detail::callback0_impl_t<boost::unit_test::ut_detail::unused, void (*)()>::invoke (this=0x10ef510)
    at /usr/include/boost/test/utils/callback.hpp:89
#8  0x00007ffff6825cb1 in ?? ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#9  0x00007ffff6805996 in boost::execution_monitor::catch_signals(boost::unit_test::callback0<int> const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#10 0x00007ffff68061b3 in boost::execution_monitor::execute(boost::unit_test::callback0<int> const&) () from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#11 0x00007ffff6825de2 in boost::unit_test::unit_test_monitor_t::execute_and_translate(boost::unit_test::test_case const&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#12 0x00007ffff680d09e in boost::unit_test::framework_impl::visit(boost::unit_test::test_case const&) () from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#13 0x00007ffff68434cb in boost::unit_test::traverse_test_tree(boost::unit_test::test_suite const&, boost::unit_test::test_tree_visitor&) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#14 0x00007ffff68089f6 in boost::unit_test::framework::run(unsigned long, bool) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#15 0x00007ffff6824287 in boost::unit_test::unit_test_main(bool (*)(), int, char**) ()
   from /usr/lib/x86_64-linux-gnu/libboost_unit_test_framework.so.1.58.0
#16 0x000000000088a227 in main (argc=1, argv=0x7fffffffe348)
    at /usr/include/boost/test/unit_test.hpp:59

blattms · 2018-08-23T12:21:41Z

Just a small note:
I did look into this more, but it seems like this is cannot be fixed easily as some of the code in GridHelpers relies on references that can be turned into pointers (e.g. for the cell centers).

Geometry is lightweight enough and this is what DUNE's grid interface mandates.

blattms · 2019-10-17T14:29:09Z

This could actually work now since the failing code is no more. Will rebase and retry.

blattms · 2019-10-17T14:59:03Z

jenkins build this please

blattms · 2019-10-30T13:07:46Z

This will not make it to the release as we have to touch a lot of code relying on this and that is bound to break easily.

SoilRos · 2025-06-13T08:58:59Z

I didn't see this one before pushing #885, sorry about that. It could be that the lifetime issues got resolved with the introduction of the shared pointers. So it would be worth to try this tests again with the latest code.

blattms · 2025-07-17T11:26:18Z

I am closing this one as there is newer and better work.

blattms mentioned this pull request Aug 2, 2018

Fix dangling reference as cell returns a copy of geometry now. OPM/opm-upscaling#261

Closed

blattms closed this Aug 3, 2018

blattms force-pushed the entity-return-copy-geometry branch from b29accb to 4d17438 Compare August 3, 2018 17:22

blattms mentioned this pull request Aug 3, 2018

Revert accidental commits #341

Merged

blattms reopened this Aug 3, 2018

blattms force-pushed the entity-return-copy-geometry branch from e7aa2af to 84593f2 Compare August 3, 2018 17:46

dr-robertk added this to the Release 2019.10 milestone Oct 1, 2019

Let Entity return geometry as a copy.

ca06210

Geometry is lightweight enough and this is what DUNE's grid interface mandates.

blattms force-pushed the entity-return-copy-geometry branch from 84593f2 to ca06210 Compare October 17, 2019 14:30

blattms removed this from the Release 2019.10 milestone Oct 30, 2019

blattms closed this Jul 17, 2025

Uh oh!

Conversation

blattms commented Jun 19, 2018

Uh oh!

blattms commented Jun 19, 2018

Uh oh!

atgeirr commented Jun 25, 2018

Uh oh!

atgeirr commented Jun 25, 2018

Uh oh!

atgeirr commented Jun 28, 2018

Uh oh!

atgeirr commented Aug 1, 2018

Uh oh!

blattms commented Aug 1, 2018 via email

Uh oh!

atgeirr commented Aug 1, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

blattms commented Aug 2, 2018

Uh oh!

atgeirr commented Aug 2, 2018

Uh oh!

blattms commented Aug 3, 2018

Uh oh!

atgeirr commented Aug 3, 2018

Uh oh!

blattms commented Aug 3, 2018

Uh oh!

atgeirr commented Aug 3, 2018

Uh oh!

blattms commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blattms commented Aug 3, 2018

Uh oh!

blattms commented Aug 9, 2018

Uh oh!

akva2 commented Aug 9, 2018

Uh oh!

akva2 commented Aug 9, 2018

Uh oh!

akva2 commented Aug 9, 2018

Uh oh!

blattms commented Aug 23, 2018

Uh oh!

blattms commented Oct 17, 2019

Uh oh!

blattms commented Oct 17, 2019

Uh oh!

blattms commented Oct 30, 2019

Uh oh!

SoilRos commented Jun 13, 2025

Uh oh!

blattms commented Jul 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

blattms commented Aug 3, 2018 •

edited

Loading