libMesh: A C++ Finite Element Library

Firstly, we heavily use this option - we _never_ renumber (when possible -
it's not possible with ParallelMesh for instance).

Secondly: Please, for the love of god, can we get rid of this being a
double negative?

I vote for:

MeshBase::allowNodeAndElemRenumbering(bool)

Or the like...

Derek

On Tue, Oct 16, 2012 at 7:43 AM, Roy Stogner <roystgnr@...:

>
> We currently have only one way to tell a mesh not to do renumbering:
> pass in "true" to MeshBase::prepare_for_use().  Would it be reasonable
> to replace this API with a direct public accessor for
> MeshBase::_skip_renumber...?  That way users would be able to control
> renumbering even when the method calling prepare_for_use() has no way
> to know a priori whether renumbering is desireable or not.
>
> The new API could either be made
> (1) simple (take away the argument to prepare_for_use(), callers can
> use the accessor instead)
> (2) backwards-compatible (prepare_for_use() still takes an argument,
> and then renumbering is skipped if either the argument or the initial
> _skip state is true)
> (3) backwards-compatible with behavior (2) at first, but increasingly
> simple (the argument is eventually libmesh_deprecated(), then removed)
> after a release or two.
>
> My inclination is towards (3) but I could be talked into (1) if
> nobody's directly using that argument (outside library code).
>
> Thoughts, preferences?
> ---
> Roy
>
>
> ------------------------------------------------------------------------------
> Don't let slow site performance ruin your business. Deploy New Relic APM
> Deploy New Relic app performance management and know exactly
> what is happening inside your Ruby, Python, PHP, Java, and .NET app
> Try New Relic at no cost today and get our sweet Data Nerd shirt too!
> http://p.sf.net/sfu/newrelic-dev2dev
> _______________________________________________
> Libmesh-devel mailing list
> Libmesh-devel@...
> https://lists.sourceforge.net/lists/listinfo/libmesh-devel
>

On Tue, 16 Oct 2012, Derek Gaston wrote:

> Firstly, we heavily use this option - we _never_ renumber (when
> possible - it's not possible with ParallelMesh for instance).

I'll take that as another vote for "don't remove the existing option
until the new option has been available for a couple releases".

> Secondly: Please, for the love of god, can we get rid of this being a double negative?
> I vote for:
> 
> MeshBase::allowNodeAndElemRenumbering(bool)

The reason for the double negative is to emphasize that renumbering is
the default, but that's a weak reason.  Unless someone else disagrees
I'll change it to "allow_renumbering" or some such shortly.
---
Roy

On Tue, Oct 16, 2012 at 10:01 AM, Roy Stogner <roystgnr@...:

> I'll take that as another vote for "don't remove the existing option
> until the new option has been available for a couple releases".


Actually - it's fine with me if you want to change it immediately.  I hate
not renumbering by passing True ;-)


> The reason for the double negative is to emphasize that renumbering is
> the default, but that's a weak reason.  Unless someone else disagrees
> I'll change it to "allow_renumbering" or some such shortly.
>

Yeah - I'm with you... but man I've messed this up a few times over the
years!

Derek

I'm for the immediate change as well. Since I don't recall discussing this option on list with anyone other that INL, I'd say of Derek is happy in this case we are good to go.

-Ben



On Oct 16, 2012, at 11:07 AM, "Derek Gaston" <friedmud@...>> wrote:

On Tue, Oct 16, 2012 at 10:01 AM, Roy Stogner <roystgnr@...>> wrote:
I'll take that as another vote for "don't remove the existing option
until the new option has been available for a couple releases".

Actually - it's fine with me if you want to change it immediately.  I hate not renumbering by passing True ;-)

The reason for the double negative is to emphasize that renumbering is
the default, but that's a weak reason.  Unless someone else disagrees
I'll change it to "allow_renumbering" or some such shortly.

Yeah - I'm with you... but man I've messed this up a few times over the years!

Derek
------------------------------------------------------------------------------
Don't let slow site performance ruin your business. Deploy New Relic APM
Deploy New Relic app performance management and know exactly
what is happening inside your Ruby, Python, PHP, Java, and .NET app
Try New Relic at no cost today and get our sweet Data Nerd shirt too!
http://p.sf.net/sfu/newrelic-dev2dev
_______________________________________________
Libmesh-devel mailing list
Libmesh-devel@...>
https://lists.sourceforge.net/lists/listinfo/libmesh-devel

On Tue, 16 Oct 2012, Kirk, Benjamin (JSC-EG311) wrote:

> I'm for the immediate change as well. Since I don't recall discussing this option on list with
> anyone other that INL, I'd say of Derek is happy in this case we are good to go. 

MeshBase::allow_renumbering accessors are in there now; I'll wait for
Derek to let us know their application codes are updated to use it,
then I'll get rid of the prepare_for_use argument.
---
Roy

We might not be able to take a look at this until Monday, but we'll
definitely let you know.

Thanks,
Cody

On Thu, Oct 18, 2012 at 9:23 AM, Roy Stogner <roystgnr@...:

>
> On Tue, 16 Oct 2012, Kirk, Benjamin (JSC-EG311) wrote:
>
>  I'm for the immediate change as well. Since I don't recall discussing
>> this option on list with
>> anyone other that INL, I'd say of Derek is happy in this case we are good
>> to go.
>>
>
> MeshBase::allow_renumbering accessors are in there now; I'll wait for
> Derek to let us know their application codes are updated to use it,
> then I'll get rid of the prepare_for_use argument.
> ---
> Roy
>
> ------------------------------------------------------------------------------
> Everyone hates slow websites. So do we.
> Make your web apps faster with AppDynamics
> Download AppDynamics Lite for free today:
> http://p.sf.net/sfu/appdyn_sfd2d_oct
> _______________________________________________
> Libmesh-devel mailing list
> Libmesh-devel@...
> https://lists.sourceforge.net/lists/listinfo/libmesh-devel
>
>

On Thu, 18 Oct 2012, Cody Permann wrote:

> We might not be able to take a look at this until Monday, but we'll
> definitely let you know.

Oh, no rush - recall that my original plan was "remove the
prepare_for_use argument after a release or two".

While you're looking at semi-internal APIs, though, a more urgent
question: are you codes using any of the DofMap constraints generation
APIs (i.e. create_dof_constraints(), process_constraints(),
add_constraint_row())) in any "unexpected ways" (i.e. other than via
System::init_data() via EquationSystems::reinit(), and/or within
System::user_constrain())?  I've found a ParallelMesh AMR bug that's
not going to be fixable without at least adding an argument to
DofMap::process_constraints(), and if I've got to mess with the DofMap
guts anyway, I might take the opportunity to refactor things to leave
those guts less exposed.
---
Roy

Go ahead and check in your change for renumbering.  We'll fix our stuff afterward.

As for constraints - we don't use any of those functions.

Thanks for keeping us in mind on these changes!

Derek

On Oct 18, 2012, at 9:45 AM, Roy Stogner wrote:

> 
> On Thu, 18 Oct 2012, Cody Permann wrote:
> 
>> We might not be able to take a look at this until Monday, but we'll
>> definitely let you know.
> 
> Oh, no rush - recall that my original plan was "remove the
> prepare_for_use argument after a release or two".
> 
> While you're looking at semi-internal APIs, though, a more urgent
> question: are you codes using any of the DofMap constraints generation
> APIs (i.e. create_dof_constraints(), process_constraints(),
> add_constraint_row())) in any "unexpected ways" (i.e. other than via
> System::init_data() via EquationSystems::reinit(), and/or within
> System::user_constrain())?  I've found a ParallelMesh AMR bug that's
> not going to be fixable without at least adding an argument to
> DofMap::process_constraints(), and if I've got to mess with the DofMap
> guts anyway, I might take the opportunity to refactor things to leave
> those guts less exposed.
> ---
> Roy

On Thu, 18 Oct 2012, Derek Gaston wrote:

> Go ahead and check in your change for renumbering.  We'll fix our stuff afterward.

I'll break your stuff by Monday, then.  ;-)

> As for constraints - we don't use any of those functions.
>
> Thanks for keeping us in mind on these changes!

No problem!

I wonder if we should find or invent some standard way of specifying
what's an "internal" vs. "external" API.  public/protected/private
doesn't quite cut it, since we have lots of methods (like those
constraints functions) that need to be called from other libMesh
components but aren't designed for user access.  I'm not sure I'd want
to *prohibit* application code from calling such functions (although
that one Ulrich Drepper paper suggests minor speed improvements from
hiding symbols) but it might be nice to easily indicate which APIs we
wrote with users in mind vs which we consider risky to call from app
code.
---
Roy

Hmmm - something is kind of weird here.  It looks like you've left in the
prepare_for_use() argument to skip_renumber... but it doesn't work.  If I
call prepare_for_use(true) I get renumbering.  If, right before that, I
call allow_renumbering(false) then everything works fine.  Does that make
sense?

I vote for just removing the argument to prepare_for_use() at this point.
 Leaving it there caused our stuff to break because it wasn't a compile
error... but it doesn't do what it used to do.

Derek

On Thu, Oct 18, 2012 at 11:28 AM, Roy Stogner <roystgnr@...:

>
> On Thu, 18 Oct 2012, Derek Gaston wrote:
>
>  Go ahead and check in your change for renumbering.  We'll fix our stuff
>> afterward.
>>
>
> I'll break your stuff by Monday, then.  ;-)
>
>
>  As for constraints - we don't use any of those functions.
>>
>> Thanks for keeping us in mind on these changes!
>>
>
> No problem!
>
> I wonder if we should find or invent some standard way of specifying
> what's an "internal" vs. "external" API.  public/protected/private
> doesn't quite cut it, since we have lots of methods (like those
> constraints functions) that need to be called from other libMesh
> components but aren't designed for user access.  I'm not sure I'd want
> to *prohibit* application code from calling such functions (although
> that one Ulrich Drepper paper suggests minor speed improvements from
> hiding symbols) but it might be nice to easily indicate which APIs we
> wrote with users in mind vs which we consider risky to call from app
> code.
> ---
> Roy
>

On Thu, 25 Oct 2012, Derek Gaston wrote:

> Hmmm - something is kind of weird here.  It looks like you've left
> in the prepare_for_use() argument to skip_renumber... but it doesn't
> work.  If I call prepare_for_use(true) I get renumbering.

I'm baffled... If skip_renumber_nodes_and_elements is passed in as
true, then

_skip_renumber_nodes_and_elements = _skip_renumber_nodes_and_elements ||
                                     skip_renumber_nodes_and_elements;

makes the member variable true, and then

if(!_skip_renumber_nodes_and_elements)
   this->renumber_nodes_and_elements();

shouldn't call the renumbering.

Assuming I'm not just missing something obvious, could you step
through in a debugger and see which step of that is failing?
---
Roy

I think the issue is that it sets it back to the old value of skip_renumber
at the end of prepare_for_use().  This is only an issue in parallel...
where I believe the renumbering can actually happen and multiple spots.  So
basically, even though we're passing "true" to skip renumbering... that's
only being used during prepare_for_use() and then forgotten after that
point... which wasn't what the old behavior did.

Derek

On Thu, Oct 25, 2012 at 10:07 AM, Roy Stogner <roystgnr@...:

>
> On Thu, 25 Oct 2012, Derek Gaston wrote:
>
>  Hmmm - something is kind of weird here.  It looks like you've left
>> in the prepare_for_use() argument to skip_renumber... but it doesn't
>> work.  If I call prepare_for_use(true) I get renumbering.
>>
>
> I'm baffled... If skip_renumber_nodes_and_**elements is passed in as
> true, then
>
> _skip_renumber_nodes_and_**elements = _skip_renumber_nodes_and_**elements
> ||
>                                     skip_renumber_nodes_and_**elements;
>
> makes the member variable true, and then
>
> if(!_skip_renumber_nodes_and_**elements)
>   this->renumber_nodes_and_**elements();
>
> shouldn't call the renumbering.
>
> Assuming I'm not just missing something obvious, could you step
> through in a debugger and see which step of that is failing?
> ---
> Roy

On Thu, 25 Oct 2012, Derek Gaston wrote:

> I think the issue is that it sets it back to the old value of
> skip_renumber at the end of prepare_for_use().  This is only an
> issue in parallel...  where I believe the renumbering can actually
> happen and multiple spots.

Any idea where?  I can only find renumber_nodes_and_elements() calls
in MeshBase::prepare_for_use() and
UnstructuredMesh::all_{first,second}_order() - I assume the latter
aren't the problem?

> So basically, even though we're passing "true" to skip
> renumbering...  that's only being used during prepare_for_use() and
> then forgotten after that point... which wasn't what the old
> behavior did.

Didn't it?  The old behavior was that there was no member variable,
and the input bool went out of scope at the end of prepare_for_use(),
so any setting passed in must have been forgotten afterwards.
---
Roy

On Thu, Oct 25, 2012 at 10:21 AM, Roy Stogner <roystgnr@...:
>
> Any idea where?  I can only find renumber_nodes_and_elements() calls
> in MeshBase::prepare_for_use() and
> UnstructuredMesh::all_{first,**second}_order() - I assume the latter
> aren't the problem?


Looks like contract() getting called from reinit() is the culprit:

0: 0   libmesh.dylib                       0x000000010674a14b
libMesh::print_trace(std::ostream&) + 43
1: 1   libmesh.dylib                       0x00000001069e8bcd
libMesh::SerialMesh::renumber_nodes_and_elements() + 35
2: 2   libmesh.dylib                       0x0000000106a0410f
libMesh::UnstructuredMesh::contract() + 317
3: 3   libmesh.dylib                       0x0000000106ab81a3
libMesh::EquationSystems::reinit() + 383
4: 4   libmoose-opt.dylib                  0x000000010634e9c2
FEProblem::initialSetup() + 2654
5: 5   libmoose-opt.dylib                  0x00000001063f64c4
Transient::execute() + 28
6: 6   libmoose-opt.dylib                  0x00000001063953fc
MooseApp::runInputFile() + 818
7: 7   libmoose-opt.dylib                  0x00000001063959df
MooseApp::parseCommandLine() + 1173
Stack frames: 11
0: 0   libmesh.dylib                       0x0000000106d2714b
libMesh::print_trace(std::ostream&) + 43
1: 1   libmesh.dylib                       0x0000000106fc5bcd
libMesh::SerialMesh::renumber_nodes_and_elements() + 35
2: 2   libmesh.dylib                       0x0000000106fe110f
libMesh::UnstructuredMesh::contract() + 317
3: 3   libmesh.dylib                       0x00000001070951a3
libMesh::EquationSystems::reinit() + 383
4: 4   libmoose-opt.dylib                  0x000000010692e9c2
FEProblem::initialSetup() + 2654
5: 5   libmoose-opt.dylib                  0x00000001069d64c4
Transient::execute() + 28
6: 6   libmoose-opt.dylib                  0x00000001069753fc
MooseApp::runInputFile() + 818
7: 7   libmoose-opt.dylib                  0x00000001069759df
MooseApp::parseCommandLine() + 1173
8: 8   libmoose-opt.dylib                  0x00000001069750a8
MooseApp::run() + 84
9: 9   moose_test-opt                      0x00000001060bb131 main + 65
10: 8: 8   libmoose-opt.dylib                  0x00000001063950a8
MooseApp::run() + 84
9: 9   moose_test-opt                      0x0000000105ad5131 main + 65
10: 10  moose_test-opt                      0x0000000105a84ca4 start + 52
10  moose_test-opt                      0x000000010606aca4 start + 52




> Didn't it?  The old behavior was that there was no member variable,
> and the input bool went out of scope at the end of prepare_for_use(),
> so any setting passed in must have been forgotten afterwards.
>

Nope - I made a change a while back that would remember the value you
passed in to prepare_for_use() and use that value for all subsequent calls
to renumber_nodes_and_elements().  Check out prepare_for_use() in r6180 for
instance...

Derek

On Thu, 25 Oct 2012, Derek Gaston wrote:

> I vote for just removing the argument to prepare_for_use() at this point.

In case I'm accidentally giving the wrong impression:

I'm not really trying to push back against this vote, just trying to
understand what's happening first.  In my experience (especially with
ParallelMesh!), trying to wallpaper over "Roy doesn't understand why
the code is doing X" with a fix for symptom X sometimes means I'm
missing an underlying bug that will crop up elsewhere later.
---
Roy

On Thu, 25 Oct 2012, Derek Gaston wrote:

> Looks like contract() getting called from reinit() is the culprit:

Thanks!

> >       Didn't it?  The old behavior was that there was no member variable,
> > and the input bool went out of scope at the end of prepare_for_use(),
> > so any setting passed in must have been forgotten afterwards.
> 
> Nope - I made a change a while back that would remember the value
> you passed in to prepare_for_use() and use that value for all
> subsequent calls to renumber_nodes_and_elements().  Check out
> prepare_for_use() in r6180 for instance...

Ah - I'd randomly grabbed an older version to refresh my memory about
the old behavior.


Okay, then, here's my preference:

Change prepare_for_use(true) save that "true" permanently, in case
there are other codes like yours which depend on the old behavior.
Add a libmesh_deprecated() whenever it sees a "true" passed in to give
any such users fair warning that they should switch to the new
allow_renumbering() accessor.

Change UnstructuredMesh::copy_nodes_and_elements() to use the new
accessor for its prepare-without-renumbering, and afterward re-set the
copy's renumbering setting to match the original mesh.  (AFAIK this is
the only use of prepare_for_use(true) in library code)).

Leave things that way until the next official release.

Remove the argument from prepare_for_use right after the release.


Sound okay?
---
Roy

This all sounds good to me!  I just want to make sure that no one is
(silently) having their programs break.  We got a bit lucky here... we had
one set of tests using a specific mesh that was hand-built in Cubit
(meaning it has a weird node numbering)... and they just happened to
fail... but only in parallel.  If not for those tests, I wouldn't have
noticed...

Derek

On Thu, Oct 25, 2012 at 11:01 AM, Roy Stogner <roystgnr@...:

>
> On Thu, 25 Oct 2012, Derek Gaston wrote:
>
>  Looks like contract() getting called from reinit() is the culprit:
>>
>
> Thanks!
>
>
>  >       Didn't it?  The old behavior was that there was no member
>> variable,
>> > and the input bool went out of scope at the end of prepare_for_use(),
>> > so any setting passed in must have been forgotten afterwards.
>>
>> Nope - I made a change a while back that would remember the value
>> you passed in to prepare_for_use() and use that value for all
>> subsequent calls to renumber_nodes_and_elements().  Check out
>> prepare_for_use() in r6180 for instance...
>>
>
> Ah - I'd randomly grabbed an older version to refresh my memory about
> the old behavior.
>
>
> Okay, then, here's my preference:
>
> Change prepare_for_use(true) save that "true" permanently, in case
> there are other codes like yours which depend on the old behavior.
> Add a libmesh_deprecated() whenever it sees a "true" passed in to give
> any such users fair warning that they should switch to the new
> allow_renumbering() accessor.
>
> Change UnstructuredMesh::copy_nodes_**and_elements() to use the new
> accessor for its prepare-without-renumbering, and afterward re-set the
> copy's renumbering setting to match the original mesh.  (AFAIK this is
> the only use of prepare_for_use(true) in library code)).
>
> Leave things that way until the next official release.
>
> Remove the argument from prepare_for_use right after the release.
>
>
> Sound okay?
> ---
> Roy

Should we also take the skip_renumbering argument away from
UnstructuredMesh::read() ?
---
Roy

I think so - you should just set allow_renumbering after you create the
Mesh object but before you do anything else...

Derek

On Thu, Oct 25, 2012 at 11:25 AM, Roy Stogner <roystgnr@...:

>
> Should we also take the skip_renumbering argument away from
> UnstructuredMesh::read() ?
> ---
> Roy
>

On Thu, 25 Oct 2012, Derek Gaston wrote:

> This all sounds good to me!

Try the attached patch?
---
Roy

After fixing this compile error:

src/mesh/unstructured_mesh.C: In member function ‘virtual void
libMesh::UnstructuredMesh::copy_nodes_and_elements(const
libMesh::UnstructuredMesh&)’:
src/mesh/unstructured_mesh.C:180: error: base operand of ‘->’ has
non-pointer type ‘const libMesh::UnstructuredMesh’

It works... meaning that if I don't call any of the new "allow" functions
my tests still work... and all of the rest of our tests work too.

So I think it's good (once you fix that compile error!) ;-)

Derek



On Thu, Oct 25, 2012 at 11:35 AM, Roy Stogner <roystgnr@...:

>
> On Thu, 25 Oct 2012, Derek Gaston wrote:
>
>  This all sounds good to me!
>>
>
> Try the attached patch?
> ---
> Roy