yade-dev team mailing list archive

[Janek Kozicki <janek_listy@xxxxx>]

Hi, I didn't see the reply from Vasileios on the mailing list. I see
it only now in quote in the reply from Boon. I will answer to both.


Chia Weng Boon said:     (by the date of Mon, 4 Feb 2019 14:55:50 +0800)

> I think in the PotentialParticles code, the "vector" parameters in the
> shape class can be removed, because they are independent of the block
> generation code.

Good, this cleaning is a good idea. I prefer that you do this in a WIP:
(Work In Progress) branch on gitlab, if possible.

 
> I still think PotentialBlocks and PotentialParticles should be
> maintained as separate codes. This is my slight preference.

I understand to you are used to how you did write that. However
duplicating code is still a source of bugs. What if you suddenly find
a bug in some of the duplicated lines and forgot to fix it in the
other one? Ok, you won't forget, but what about future users of this
code? They will will not even be aware of those duplicated lines that
need a parallel fix of the same thing.

Try comparing those files with meld. In meld preferences click four checkboxes:

Preferences -> Text Filters ->
(1) Trim blank lines
(2) C++ comment
(3) C comment
(4) All whitespace

Then run:

meld ./pkg/dem/KnKsLaw.hpp ./pkg/dem/KnKsPBLaw.hpp
meld ./pkg/dem/Ig2_PP_PP_ScGeom.cpp
meld ./pkg/common/Gl1_PotentialBlock.cpp ./pkg/common/Gl1_PotentialParticle.cpp
meld ./pkg/dem/PotentialParticle2AABB.cpp ./pkg/dem/PotentialBlock2AABB.cpp

(maybe there are more and I just missed it)

You will see that those files are nearly identical. The only
difference is formatting, whitespaces and newlines. This is exactly a
place where bugs live and multiply.

Would be much better if you derive one class from the other and
instead of duplicates call the code in parent class to do the stuff
which is the same for both classes. In particular I saw that those
functions are nearly identical:
::BrentZeroSurf  ::evaluatePB  ::getNormal  ::getPtOnParticle2  ::customSolve
but there are others.


> Also, as PotentialParticle runs fine without MOSEK, should we erase
> the part calling MOSEK?  It is more robust, and it is like a safety
> net (but because it is calling an external library, which has
> another layer of pre-processing, it is slower). Just curious about
> GPL compatibility.  I can't use it now because I am not an
> academic, and I am using more of PotentialBlocks.

It is up to you.


> The reason why the physical contact properties were stored in the Shape
> file (rather than Material) is because they were originally developed for
> rock mechanics problems, where each face will have a different physical
> property.The Potential Block contact detection algorithm is compatible with
> the block generation algorithm here:
> https://www.sciencedirect.com/science/article/pii/S0266352X14002122

ah, I see now. Maybe there is a way to move it to Material somehow.
But I see the difficulty now. So that's something to thing about.



> On Mon, Jan 28, 2019 at 12:24 AM Vasileios Angelidakis (PGR) <
> V.Angelidakis2@xxxxxxxxxxxxxxx> wrote:  
> 
> > Hi,
> >
> >
> > Janek thanks for the in-depth remarks on the PB (PotentialBlock) & PP
> > (PotentialParticle) codes. It will be very useful to have a discussion on
> > the matter and try to improve them.

you are welcome, please make sure that your reply goes to yade-dev
next time. I might have never see it!


> > Regarding the compilation warnings that stem from unused variables, I can
> > help suppress them by commenting out the unused variables, if Chia can
> > verify with me that he does not intend to use any them in the near future.
> > For the other warnings, I will track them inside the code and try to
> > suppress them as well. I will let you know if I find any difficulties.

great. The warnings are cleared now :)


> > Personally, I believe that we could unify some classes of the PB & PP,
> > since a part of the code is overlapping; although some parts of the codes
> > are completely different. Some comments on this -I see Chia just discussed
> > some of these matters earlier today-. (Chia please correct me if I have
> > misinterpreted something):

yes, unification will solve all the problems in code :)

> >    - The distinction between the two codes, conceptually, is that the
> >    PotentialBlocks have flat faces with no curvature, while the
> >    PotentialParticles can have curved faces and look like "cushions" (I liked
> >    that reference!). This curvature of the latter is controlled by the
> >    parameters "k" and "R" in the PotentialParticle shape class.

ok. So this difference appears in some places in code. Make those
places into separate functions, and put duplicated code in parent
class :)

> >    Also, the PotentialBlocks code has been developed to incorporate some
> >    more parameters and to be compatible with the BlockGen code, in which a
> >    PotentialBlock can be subdivided to more blocks, by intersecting the
> >    particle with discontinuity planes.

ok, good.

> >    You will find in the source code that "R" is used in some parts of the
> >    PB code as well, but as a reference length unit and not as the "radius of a
> >    shadow particle", as it is in the PP class. So, in a case where the PB and

so perhaps use different names for variables to avoid mixing them ;)

> >    PP are unified, one should choose "k=0", in order to get a particle with
> >    flat faces (like the PB) or "k>0", in order to get a particle with curved
> >    faces (like the PP). I should clarify, that when using the PB code, even if
> >    a non-zero value is given for "k", it is not used; the faces remain flat. I
> >    guess it just stayed in the code, to maintain some architectural similarity
> >    between the PB & PP classes.

ok :)

> >    - The most important distinction between the PB and PP codes from a
> >    programming standpoint is the contact detection algorithms used, as, in the
> >    former contact is established using linear programming (and CLP software),
> >    while in the latter contact is established using conical optimisation
> >    (either with MOSEK or Chia's second order code).

ok, so if the contact detection is different then why so many lines in

meld ./pkg/dem/Ig2_PP_PP_ScGeom.cpp ./pkg/dem/Ig2_PB_PB_ScGeom.cpp

are the same? Looks like you still could extract some common stuff into a parent class.

> >    So, if we were to merge the Shape (PotentialBlock &
> >    PotentialParticle), Bound (PotentialBlock2AABB & PotentialParticle2AABB), a
> >    distinction could be made regarding the IPhys functor, so that, when k=0

yes! And in fact when preparing Engines in the .py script you decide
which functors to use. And the derived functor will use common code
from parent functor.

> >    (PB particles) we use the Ig2_PB_PB_ScGeom &
> >    Ip2_FrictMat_FrictMat_KnKsPBPhys functors, while if k>0 (curved
> >    faces), we use the Ig2_PP_PP_ScGeom & Ip2_FrictMat_FrictMat_KnKsPhys
> >    functors, respectivelly. As Chia said, I'm not sure we can unify the IGeom
> >    functors, as they were developed under different conceptions; though, new
> >    ideas are welcome :) .

not everything is similar. Keep what is different in the class where it belongs to :)

> >    I am interested to improve these codes, since I currently use the
> >    PotentialBlock class for my PhD. I don't think that I have the technical
> >    knowledge to apply these changes by myself. Maybe we can achieve this
> >    with some periodic consulting from Chia and some help from you Janek? Send
> >    me an email if you have the time to assist on this, I'm happy to discuss.

yeah, we can discuss. I will have a look at your commits and you will
make tests to make sure that his "revolution" does not damage the
code ;)

> >    Chia and Janek, do you think it would be of value to merge just part
> >    of the codes, i.e. only the Shape and Bound classes for now, to simplify
> >    things?

ok. Better do this in a WIP: branch, and not merge it until it is
ready. Because that's a major refactoring. To kip your branch
up-to-date with master you will have from time to time to call:
 pull --rebase upstream master 
:)


> >    - Regarding the the "heat-capacity" parameter, I'm afraid I
> >  can't help much, as I am not familiar with the topic.

That looks like a Material parameter to me, (a new class derived from
Material). Unless it changes for each body during simulation, then
it's a State parameter (a new class derived from State).


> > These are some thoughts on my part. I'm very interested to hear
> >  more from you Janek, as you know YADE through and through and
> >  from Chia, as he is the original developer of all this work and
> >  I believe he can give us a better understanding of the
> >  conceptual design of the codes and if he thinks we should merge
> >  them.

yeah, lets discuss. Did I miss any of your communication? Like
something you have written went unanswered?

> > I will go through with removing the warnings during the next days
> >  & I will also be sending a piece of documentation in .rst format
> >  regarding these codes soon.

great, thanks!



sorry for even longer post :)
Janek


----------------------------


> >
> >
> > Sorry for the long post.
> > Best Regards,
> > Vasileios











> >
> > ------------------------------
> > *From:* Yade-dev <yade-dev-bounces+v.angelidakis2=  
> > ncl.ac.uk@xxxxxxxxxxxxxxxxxxx> on behalf of Chia Weng Boon <
> > chiaweng@xxxxxxxxx>  
> > *Sent:* 27 January 2019 15:03:54
> > *Cc:* Yade developers
> > *Subject:* Re: [Yade-dev] Potential Blocks and Potential Particles -
> > brief analysis of the Object Oriented C++ source code
> >
> > Hi Janek,
> > Potential Blocks and Potential Particles are different algorithms.
> > Potential Blocks (PB) were developed for polygonal particles "without the
> > spherical term".  Potential Particles (PP) were developed for general
> > convex non-spherical particles.
> >
> > The PB algorithm is based on linear programming (solved by calling linear
> > programming libraries, now CLP ), and the contact point is calculated as
> > the analytic centre.
> >
> > The PP algorithm requires solving a nonlinear constrained optimisation
> > problem.  The code uses second order coning programming, where there are
> > two options.  The first is to call the function I wrote, or to call MOSEK
> > (only free for academics).  PB is faster than PP, since PB is easier to
> > solve.
> >
> > The similar points between PB and PP are the way the contact normal is
> > calculated (PB without the spherical term), and the way the overlap
> > distance is calculated.
> >
> > The reason why the physical contact properties were stored in the Shape
> > file (rather than Material) is because they were originally developed for
> > rock mechanics problems, where each face will have a different physical
> > property.The Potential Block contact detection algorithm is compatible with
> > the block generation algorithm here:
> > https://www.sciencedirect.com/science/article/pii/S0266352X14002122
> >
> > Vasileios and myself were in touch.  From what I know, he's using the
> > PotentialBlock section (and also extracting the display with ParaView).  I
> > agree there are many redundant variables that need to be removed in order
> > to avoid confusion, and keen users may help greatly.  While I am much less
> > active, I am still using YADE very occasionally as a hobby, and would like
> > to see the algorithms get integrated better (eg display in QT, periodic
> > boundaries, etc).  Hopefully in the spirit of open source, interested users
> > will continue with this effort, leading to more possibilities.
> >
> > Boon
> >
> >
> > On Sat, Jan 26, 2019 at 11:02 PM Janek Kozicki <janek_listy@xxxxx> wrote:
> >
> > Hi,
> >
> > I looked a bit more into PotentialBlocks, and there are some things
> > which surprise me, there seem to be some mistakes.
> >
> > 1. we strive to remove all compiler warnings. Currently when yade is
> > compiled without PotentialBlocks, there are only 6 warnings, and
> > four of them are caused by external libraries. Problem is that when
> > compiling with PotentialBlocks there are 198 warnings, especially
> > about unused variables and narrowing conversion. I've filed a
> > respective bug report about that:
> > https://bugs.launchpad.net/yade/+bug/1813401
> >
> > 2. material parameters (like stiffness or heat capacity) are stored
> > inside Shape class PotentialBlock.hpp. But they should be stored
> > inside a class derived from Material. My impression (I might be
> > wrong, in that case please correct me) is that PotentialParticle is
> > using default materials and default contact law. Then later you
> > decided that you need a more customized Material (e.g. with heat
> > capacity) and you have cloned PotentialParticle into PotentialBlock,
> > instead of adding a Material class. This error propagates down
> > through all my next remarks.
> >
> > 3. As a result of above mistake there is no IPhys (interaction
> > physics) functor. Its main purpose  is to calculate the physical
> > parameters of the contact. Eg. the heat capacity of two
> > PotentialBlocks that are in contact. Its contents was put into IGeom
> > functor Ig2_PB_PB_ScGeom.cpp
> >
> > 4. Ig2_PB_PB_ScGeom.cpp and Ig2_PP_PP_ScGeom.cpp are the duplicates of
> > each other. The code that is a difference between them is about
> > physics and would better go to the IPhys functor. Please do not
> > duplicate code. Duplicate code is the worst source of all bugs.
> > Because someone after a long debugging session finally corrects some
> > mistake, and believes that it has been corrected. And later the same
> > bug resurfaces in the duplicated part of the code.
> >
> > 5. PotentialBlock.hpp and PotentialParticle.hpp are duplicates of each
> > other, the difference between them is what would better go into
> > Material class
> >
> > 6. because they are duplicates the drawing of class PotentialBlock is
> > not implemented, while it would work perfectly well with
> > Gl1_PotentialParticle functor
> >
> > I am sorry that my remarks are so fundamental in a sense. But this is
> > all in good will, I hope that fixing these problems would make the
> > code much more easier to work with for others, and much less prone to
> > bugs.
> >
> > After you make a class derived from Material to handle
> > PotentialBlocks properly, you will see that it will work best with
> > interaction physics (IPhys) functor which handlles this material. And
> > suddenly there is no need to duplicate code from
> > PotentialParticle.hpp into PotentialBlock.hpp. There will be only
> > PotentialParticle which sometimes can use the newly created material
> > and also will work with the default material. And this will cause
> > Gl1_PotentialParticle functor to draw correctly "both" of them.
> > "Both" in quotes, because there in fact will be inly one, but with
> > different material.
> >
> > I hope that you can see that removing a duplicated code will suddenly
> > make it all simpler.
> >
> > I understand that you stopped working on this topic and others are
> > using this code. Maybe Vasileios will want to fix this? In any case
> > it is good to discuss this, and if possible try to correct it.
> >
> > After some discussion, maybe we should turn this Object Oriented
> > design problems into a bug report about PotentialBlcoks.
> >
> > best regards
> > --
> > Janek Kozicki                               http://janek.kozicki.pl/  |
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~yade-dev
> > Post to     : yade-dev@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~yade-dev
> > More help   : https://help.launchpad.net/ListHelp
> >
> >  


-- 
Janek Kozicki                               http://janek.kozicki.pl/  |