Overloading of NodeAtCoordinate

[vhirtham]

The NodeAtCoordinate function of our MeshFEM class is currently overloaded with two versions. If you just pass the coordinates, you will get the coordinate nodes. If you throw in a additional dof, you get a dof Node. Since the stupid user (me) is still used to the old approach with only one node storing all the dof values I find the overloading here a little bit disadvantageous. I wanted to add a new Dof-Element to my mesh that uses already existing nodes. I did this:

leftElementCollection.AddDofElement(
            dofWV, ElementFem({&mMesh.NodeAtCoordinate(coordLeftBoundary)}, boundaryInterpolation));

where leftElementCollection was just a reference to the corresponding element Collection. Did compile, but crashed somewhere on the way because of some missing instances (new feature) because I missed to pass the dof. The whole time I was on the wrong trail while searching for the errors origin, cause the function call didn't look obviously wrong to me.

So maybe we could rename the functions to CoordinateNodeAtCoordinate and DofNodeAtCoordinate to make it a little bit clearer. There might be other and better solutions, but I am lacking the big picture to tell which is the best approach.

[TTitscher]

I had this issue myself several times. Mostly, when it comes to constraints and I accidentally try to constrain coordinate nodes. I added warnings for those cases. But I believe that we should solve that with a type separation of

CoordinateNode	DofNode
has coordinates	has multiple instances of dof values
--	has a dof numbering
--	can be constrained

and similarly CoordinateElementInterface and DofElementInterface. This will make other things more clear. You cannot accidentally build a jacobian from a displacement element. I am not sure about the design though. Mutliple inheritance?

ElementInterface
  ::Interpolate() = 0
  ::N() / dNdXi()

CoordinateElement : ElementInterface
   ::ExtractCoordinates()

CoordinateElementFem : CoordinateElement
   ::GetNumNodes()
   ::GetNode()

CoordinateElementIga : CoordinateElement
   ::GetControlPoints()
   ::??

DofElement : ElementInterface
   ::ExtractDofValues(doftype, instance)
   ::DofNumbering()

....

Or maybe leave out ElementInterface to avoid the multiple inheritance?

With that, we get an error at compile time / linting, if, in your case, you try to create a DofElement from CoordinateNodes.

What do you think?

[vhirtham]

I thought about something similar, but did think that might be a bit too much to ask for :stuck_out_tongue:. I favor the approach with the distinct types. Errors at compile time are always better than at runtime.

[TTitscher]

might be a bit too much to ask for

Ehh, feel free to implement that. :+1:

[vhirtham]

might be a bit too much to ask for

Ehh, feel free to implement that. :+1:

I thought asking for a change in the data structure would be too much. Didn't mean I wanted somebody else to do it :stuck_out_tongue: . I ll start working on that today.

[TTitscher]

Nice! I'll be there if you need help!

[vhirtham]

I now separated coordinates and dofs in the branch NodeCoordinates. Tests are runnig without failures. However, there are some things that are not clear so far.

Function names Currently NodeCoordinates ( I should remove the "s" at the end) and NodeDof have a common set of functions which do exactly the same. Well, basically NodeDof is a NodeCoordinate with some extra functions. At the beginning I renamed all functions with "value" or "dof" to "coordinates. But that turned out to be a little bit annoying cause I would have to copy a lot of functions in other classes where the only difference between both versions is a single function call with different names. So I sticked with the "value" function names. Only thing that should be renamed in both versions is int GetDofNumber(int component) const since the name is a little bit confusing on a coordinate node. What do you think. Should the function names be more specific to the node type or should we keep the names equal to avoid writing the same functions twice. In the latter case, we might be capable of reusing the code of NodeCoordinate in NodeDof via composition.

ElementColections Previously the ElementCollectionFEM was just a typedef of a template with ElementFEM as parameter. Since I don't know if coordinate elements make sense for IGA, I copied the template class and adjusted everything for ElementFEM. I know, it could be done a lit more elegant using template voodoo, but since I don't know if IGA can also use coordinate elements I avoided the pain of messing around with templates. Maybe Peter should say something here.

There is probably more that needs to be clarified but I am a little bit in a hurry and will write the rest tomorrow, if I forgot something.

[pmueller2]

I am not sure if this is going into the right direction: The problem of accidently getting Coordinate Nodes because you forgot to provide a DofType is solved by renaming the respective functions. But why are now no coordinate nodes allowed that have velocities? Time dependent coordinates are not uncommon and now that we have the possiblity to use velocities why these restrictions? I think that coordinate nodes and elements are specialized Dof Elements and not the other way around.

[vhirtham]

Well, I just removed the instance since I didn't know about a proper use case. I can put it back in if you think it is necessary. I think the main difference between coordinate nodes and dof nodes is, that the first ones are basically constant while the others are variable. If coordinates need to be variable too, you might be right. However, if we think the coordinates and dofs shouldn't be separated, we can just delete the branch. ;)

[TTitscher]

Definitely a step in the right direction. I have a few points that mainly coincide with my post above.

The CoordinateNode (I prefer that over NodeCoordinate) and the DofNode (I prefer that over NodeSimple) have some fundamentally different features. The DofNode is alright as it is, but the CoordinateNode does not need instances and mCoordinateNumbers. It may also be advantageous to rename the methods from xyzValues to xyzCoordinates.

With those changes, it IMO becomes quite clear that the ElementFem<TNode> approach is problematic. This might also be the reason why you kept mCoordinateNumbers, right? I find a CoordinateElementInterface and DofElementInterface more suitable. And yes, this will introduce code duplication. But this can be refactored in a second step.

(@pmueller2 We currently do not support moving coordinates. So I would see your idea as a new feature rather than a refactoring. And we want to refactor here. But @potto1 simply used the displacement DOF field for his impact problems. So fixed coordinates and increasing displacments = moving body.)

[vhirtham]

The CoordinateNode (I prefer that over NodeCoordinate) and the DofNode (I prefer that over NodeSimple) have some fundamentally different features. The DofNode is alright as it is, but the CoordinateNode does not need instances and mCoordinateNumbers. It may also be advantageous to rename the methods from xyzValues to xyzCoordinates.

Totally agree here. Didn't change the xyzValues stuff because it made things more complicated (as I stated before) and my primary target for the commit was to separate both Nodes first and get everything running again. In a next step I would do the renaming and removing of unnecessary functionality on the CoordinateNode.

This might also be the reason why you kept mCoordinateNumbers, right?

Correct!

I will wait what @pmueller2 responds, before continuing. If all the remaining changes are applied, I should probably start a pull request.

[pmueller2]

Don't wait for me, just go on.

[joergfunger]

I know that we have voted (for timely reasons) against rethinking about the way we want to perform the interpolation. However, I think it would be worth rethinking about some of the design ideas we had at the very beginning, in particular the way we handle nodes and their coordinates.

• A mesh should contain keypoints (just to make the distinction to nodes with dofs), each having a coordinate. Those keypoints are connected by lines, loops of lines form surfaces and surfaces form volumes. Those lines/surfaces may also have interpolations to define curved lines/surfaces.
• dofs (nodes) are now attached to any of those entities (keypoints, lines, surfaces, volumes). A selection of the dof (nodes) should be performed via the geometric entities.

[vhirtham]

A while ago @pmueller2 and I talked about your first bullet point. I think it can be beneficial to have this kind of hierarchical structure when you want to do a little bit more with your mesh than just creating elements and start the calculation.

[TTitscher]

@joergfunger Even though I like the idea (and @pmueller2 will probably adore it :smile:), I think this is a different issue. We have several places to put those nice to have ideas. Maybe a new issue or the `neat ideas tile in PDE.