Topic: Comments from my merge (please read fellow developers)

Hi all, I'm finally got around to merge in my "recent" changes, and I have a bunch of small comments (nothing really difficult here):

0. There was issues with a few tests.

concrete_fcm_tension.in   <-- This test seems broken? One component seems to be 0.1, but test says 0. All other components check out correctly.
MPS_block0*  <-- These tests didn't even pass before merging, but it wasn't noticed because of a bug with coupled problems and error checking module
tr2shell7XFEM_CZ <-- Not sure what happened here. Seems it got broken before my merge as far as I can see.

I left MPS with input files which I think would be correct. I think the error check values simply needs to be updated based on this.


1. I see there has been work on unifying the load integration code (great!). We should continue to rename

computeBoundaryEdgeLoadVector -> computeEdgeLoadVector  (also change the load BoundaryLoad -> EdgeLoad)
set 1 elementedges XXX  -> set 1 edges XXX
set 1 elementboundaries XXX  -> set 1 surfaces XXX

and I'm probably forgetting some other places right now. We could support both names for a time if we wish to have some backwards compatibility.


2. I recommend that all tests move their definition of sets to directly follow the elements. Doing this we have all the geometric information (nodes, elements, sets), in one continuous section (which can be easily combined with the @include statement).
Both positions of sets are currently supported, but I think the goal should be to deprecate the old position at the bottom of the file.


3. When it comes to load-integration, i believe that some code is in fact wrong, or just misleading; I'd really want to call an EdgeLoad to be a "force/length", and a SurfaceLoad to be a "force/area", and a BodyLoad a "force/volume".
The corner cases I've seen have been using 2D cases with unit thickness anyway, so there would not really be much difference, but I think keeping the units well defined in the input file is hugely important.
The notation (and also the code in some places), do not follow this, for example this code:

double dV = this->computeEdgeVolumeAround(gp, boundary);

in the transportelement has 2 problems: what this method returns is actually an area (not a volume), and it's used in the edge load integration, so it should not have included the thickness (which is already part of the load).
If we simply keep the definition of EdgeLoad = N/m then i think it's easy to get it all consistent.

4. I have merged some new code for StaticStructural so that it now supports reference loads (CALM). The implementation I have done simply marks some loads as "reference" in the input file. It completely avoids using load increments, thus supports all kinds of re-meshing, xfem/dynamic lagrange mult., dynamic time-stepping, etc.
I also think it's much easier to use, and will also let you apply both a fixed and reference load in 1 step.
It's not a 1 to 1 replacement of the nlinearstatic implementation though (intentionally). Especially the case where multiple steps of meta steps (switching from direct to indirect control). I can't even figure out when you would ever want something like that. In my implementation, the reference load stays a reference load forever. I have some ideas of how to achieve switching (fixating , but I'm not even sure this is at all desirable, and there are numerous issues with any such approach (self modifying input descriptions can cause a lot of pain).

Re: Comments from my merge (please read fellow developers)

Hi Mikael, we tried to compile it, but there are many errors. Can you, please, look at it?

Thanks, Martin

Re: Comments from my merge (please read fellow developers)

Hi Martin,

I had forgotten to do "git add" on a whole bunch of files (I didn't see them because of the huge number of changes that *was* added in the mergetool pass)
Sorry for the noise.

Re: Comments from my merge (please read fellow developers)

Thanks, it compiles fine now.

However, these tests fail     
96 - test_staggeredsolver.in (Failed)
218 - test_tmpatch46.in (Failed)
245 - test_MPS_block01_tmsm.in (Failed)
248 - test_MPS_block02_tmsm.in (Failed)

You mentioned MPS_block0*, but do you have any idea about the other two?

Re: Comments from my merge (please read fellow developers)

I will have a closer look at staggeredsolver (I didn't see that fail, but I might have missed something).

I think tmpatch46 might just be a due to float precision?
Some of the test I had to adjust tolerances (lowering rtolf, setting miniter 1/2, or just lowering the error check tolerance).  I know I tweaked tmpatch46 during the merge. Are you getting large errors?

Re: Comments from my merge (please read fellow developers)

tmpatch46 is working now, it just uses lstype 1 and I didn't compile with iml.
Moreover, there is something weird with concrete_fcm_tension.in. It passes im my case, but you mentioned that it failed, and when Edita tried it, it failed as well.

Re: Comments from my merge (please read fellow developers)

Hi Martin,
I got the error in gcc, but not with clang.  clang gave good warnings on this, there are some uninitialized returns of "traction" in giveNormalCrackingStress.

Initializing it to zero "solves" the error, but this indicates that the method is broken, since traction should always have been initialized somewhere else. I recommend
double traction = -1.;
which reveals deeper problems (this traction goes through uninitialized).

I'd just remove the "traction" variable and directly return instead, so that it's easier to detect problems (i.e. if we ever reach the bottom of the method, there is a bug). Also saves on "break" statements, so the code is probably more readable.

We can make it consistent by initializing it to zero, but that would be a sign that it goes through the whole method without being set, which seems like a bug.