Clean up system vector and matrix initialization #1378

YaqiWang · 2017-07-06T16:48:48Z

This also enables the construction of vectors and matrices outside of the systems. Close #1376.

YaqiWang · 2017-07-06T17:00:14Z

I will not touch the part in @roystgnr 's #1377.

roystgnr · 2017-07-06T17:04:33Z

Eliminating TransientSystem::init_data once it's unused was a nice touch. We can apply that on top of my refactoring.

jwpeterson · 2017-07-06T17:05:53Z

@YaqiWang please accept my invitation to join the libmesh "Associates" team so your PRs will be tested automatically. Otherwise, every time you push I need to activate them all by hand...

YaqiWang · 2017-07-06T17:36:41Z

@jwpeterson Thanks for the invitation. I have accepted.

jwpeterson · 2017-07-06T18:33:57Z

~~Hm... sounds like you guys were not expecting this to conflict with #1377, but it does.~~

YaqiWang · 2017-07-06T18:34:20Z

yes, i am resolving it.

YaqiWang · 2017-07-06T18:42:47Z

src/systems/implicit_system.C

@@ -113,6 +107,11 @@ void ImplicitSystem::init_matrices ()
  if (matrix->initialized())
    return;

+  // Clear any existing matrices


I am not sure if I should move this code inside here. Let me try move it back. I will need to run MOOSE test to make sure. BTW, how do I run libmesh test locally?

how do I run libmesh test locally?

make check in the top level of the build directory will run all the libmesh unit tests and examples. You can also run make check within a single example directory to test it alone.

YaqiWang · 2017-07-06T20:15:55Z

I saw a Memory leak detected! in one of the examples, possibly due to system matrix management. So do not merge it.

YaqiWang · 2017-07-06T20:27:16Z

I think this is good to go. Thanks.

jwpeterson

One additional nitpick: can you please fix your commit message formatting so that it follows these guidelines.

jwpeterson · 2017-07-06T20:37:43Z

include/systems/implicit_system.h

+  /**
+   * Initializes the matrices associated with this system.
+   */
+  virtual void init_matrices ();


Maybe I didn't fully understand the new design that was discussed. Did init_matrices() need to become public?

I thought I would need this to be public. But after working on moose after this, turns out I only need to do

// do not assemble system matrix for JFNK solve if (solverParams()._type == Moose::ST_JFNK) _nl->system().set_basic_system_only(); _nl->nonlinearSolver()->jacobian = NULL;

in MOOSE FEProblemBase::init. So this does not have to be public to me. However, it may be desired in general.

I never desire to make things public without very compelling reasons....

ok, will make it back.

jwpeterson · 2017-07-06T20:42:13Z

src/systems/explicit_system.C

-  // Add the system RHS.
-  // (We must do this before initializing the System data,
-  //  then we lose the opportunity to add vectors).
+  // Restore us to a "basic" state
  this->add_system_rhs ();


Were we calling add_system_rhs() in ExplicitSystem::clear() before? If not, I find it confusing for a clear() function to be adding vectors.

Me too, but this appears necessary for restoring. It also follows @roystgnr 's coding in TransientSystem::clear.

jwpeterson · 2017-07-06T20:44:16Z

src/systems/implicit_system.C

+
+  _matrices[mat_name]->clear();
+  delete _matrices[mat_name];
+  _matrices[mat_name] = libmesh_nullptr;


This leaves a NULL entry in the _matrices container that will cause ImplicitSystem::have_matrix() to return confusing values.

It also does 3 map lookups (via operator[]) which is unnecessary.

good catch. I forgot to erase the entry in _matrices.

I know that is not ideal, I basically copied some codes in remove_vector... Want me change that as well?

Want me change that as well?

No, not for this PR. I'll fix it some other time...

Yeah, because it is in system.C, I fixed it here already. It belongs to cleanup.

jwpeterson · 2017-07-06T22:36:55Z

src/systems/implicit_system.C

+  delete pos->second;
+
+  _matrices.erase(mat_name);
+}


But this still does 3 lookups: 1 for have_matrix(), one for find() and one for erase(key). Also I don't think it's necessary to call clear() followed by delete because all the SparseMatrix destructors call clear(). So, the following should work:

matrices_iterator pos = _matrices.find (mat_name); if (pos==end) return; delete pos->second; _matrices.erase(pos);

You are right, will update.

jwpeterson · 2017-07-06T23:26:04Z

src/systems/implicit_system.C

+
+  //Return if the matrix does not exist
+  if (pos == _matrices.end())
+    return


No semi-colon here! I'm not sure how this even compiles?!

yes, strange.

The vectors and matrices are managed in the same way throughout the systems. Because adding system matrix in implicit system is moved to the constructor, users can choose to delay the matrix construction by calling set_basic_system_only(). A new interface function, remove_matrix(), is added in ImplicitSystem.

moosebuild · 2017-07-10T13:56:40Z

Job Test debug on 99cc7ab : invalidated by @jwpeterson

Two of the usual debug mode timeout suspects

jwpeterson · 2017-07-10T14:15:00Z

This looks OK to me, but @roystgnr should probably also sign off on the changes.

roystgnr · 2017-07-10T15:41:25Z

src/systems/explicit_system.C

@@ -45,6 +46,8 @@ ExplicitSystem::~ExplicitSystem ()
 {
  // clear data
  this->clear();
+
+  remove_vector("RHS Vector");


Why do we need this here? The additional vectors are all about to be destroyed by System::~System anyways.

Not sure, because System::~System calls this->clear(), which is ExplicitSystem::clear(), right? If so, rhs vector is not cleared.

Although, to be fair, that's not obvious. System::~System() calls this->clear(), and C++ rules say that that means System::clear(), not ExplicitSystem::clear(), which in this case is exactly what we want and won't leave a re-added RHS vector, but which is such obscure behavior that CERT recommends never doing it.

I see. Thanks. We then can completely remove the ExplicitSystem destructor, right?

roystgnr · 2017-07-10T15:45:16Z

src/systems/implicit_system.C

@@ -55,6 +57,8 @@ ImplicitSystem::~ImplicitSystem ()
 {
  // Clear data
  this->clear();
+
+  remove_matrix("System Matrix");


This, on the other hand, looks like the ideal workaround for what otherwise would have been a memory leak introduced by the improved ImplicitSystem::clear() behavior.

roystgnr · 2017-07-10T15:45:53Z

src/systems/system.C

@@ -713,15 +713,16 @@ NumericVector<Number> & System::add_vector (const std::string & vec_name,

 void System::remove_vector (const std::string & vec_name)
 {
+  vectors_iterator pos = _vectors.find(vec_name);
+


Thanks for the optimization

roystgnr · 2017-07-10T16:10:31Z

Verify that that remove_vector() call is redundant, and take it out (a separate commit, no need to rebase, since this isn't a breakage-inducing bug), and this should be good to merge.

roystgnr · 2017-07-10T16:11:20Z

And if you want to be super nice you could add a comment to System::~System explaining what's going on so that it doesn't confuse anyone else in the future.

roystgnr · 2017-07-10T16:12:32Z

Or you could even just change this->clear() to an explicit System::clear() there, if you feel that's clearer.

(no, I couldn't resist the pun)

YaqiWang · 2017-07-10T16:58:51Z

@roystgnr added one commit to address your comment.

roystgnr · 2017-07-10T17:19:32Z

This looks good to me, once tests are done.

YaqiWang force-pushed the system_cleanup_1376 branch from 1d6aaaf to bffee72 Compare July 6, 2017 17:04

YaqiWang force-pushed the system_cleanup_1376 branch from bffee72 to 6919d21 Compare July 6, 2017 17:34

YaqiWang force-pushed the system_cleanup_1376 branch 2 times, most recently from 4a868b8 to 91b9039 Compare July 6, 2017 18:28

YaqiWang force-pushed the system_cleanup_1376 branch from 91b9039 to 6633df6 Compare July 6, 2017 18:38

YaqiWang commented Jul 6, 2017

View reviewed changes

YaqiWang force-pushed the system_cleanup_1376 branch from 6633df6 to e4d0016 Compare July 6, 2017 20:21

jwpeterson requested changes Jul 6, 2017

View reviewed changes

YaqiWang force-pushed the system_cleanup_1376 branch 3 times, most recently from 015729a to 68ae6c1 Compare July 6, 2017 21:19

jwpeterson reviewed Jul 6, 2017

View reviewed changes

YaqiWang mentioned this pull request Jul 6, 2017

Enabling ExplicitSystem in FEProblem idaholab/moose#5288

Closed

YaqiWang force-pushed the system_cleanup_1376 branch from 68ae6c1 to aa7eb94 Compare July 6, 2017 23:04

jwpeterson reviewed Jul 6, 2017

View reviewed changes

YaqiWang force-pushed the system_cleanup_1376 branch from aa7eb94 to 99cc7ab Compare July 6, 2017 23:32

jwpeterson approved these changes Jul 10, 2017

View reviewed changes

roystgnr reviewed Jul 10, 2017

View reviewed changes

remove unnecessary destructor of ExplicitSystem libMesh#1376

a662336

roystgnr merged commit be6af92 into libMesh:master Jul 10, 2017

Clean up system vector and matrix initialization #1378

Clean up system vector and matrix initialization #1378

Conversation

YaqiWang commented Jul 6, 2017

YaqiWang commented Jul 6, 2017

roystgnr commented Jul 6, 2017

jwpeterson commented Jul 6, 2017 • edited Loading

YaqiWang commented Jul 6, 2017

jwpeterson commented Jul 6, 2017 • edited Loading

YaqiWang commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YaqiWang commented Jul 6, 2017

YaqiWang commented Jul 6, 2017

jwpeterson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moosebuild commented Jul 10, 2017

jwpeterson commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roystgnr commented Jul 10, 2017

roystgnr commented Jul 10, 2017

roystgnr commented Jul 10, 2017

YaqiWang commented Jul 10, 2017

roystgnr commented Jul 10, 2017

jwpeterson commented Jul 6, 2017 •

edited

Loading

jwpeterson commented Jul 6, 2017 •

edited

Loading