Doc of cylinder-related models

Bug #1746037 reported by Jérôme Duriez on 2018-01-29
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Yade
Undecided
Unassigned

Bug Description

Hello,

I have few questions regarding the doc of chained cylinders classes and such.

1. It seems to me the doc ("doc" string of YADE_CLASS_BASE_DOC_*) of Law2_ChCylGeom6D_CohFrictPhys_CohesionMoment [1] and Law2_CylScGeom_FrictPhys_CundallStrack [2] may deserve some improvements.
At the moment, they look very like the doc of Law2_ScGeom_FrictPhys_CundallStrack [3] even though Law2_ChCylGeom6D_CohFrictPhys_CohesionMoment does have cohesion for instance ;-)

2. I would have a similar remark (about room for improvement) for the doc of ChCylGeom6D [4]

3. Do we agree Ig2_ChainedCylinder_ChainedCylinder_ScGeom6D actually return a ChCylGeom6D instance [5] ?
And neither a ScGeom6D (as the name suggests), nor a ScGeom (as the doc [6] suggests)

4. Do we agree "ScPFaceCoGeom" actually is "ScGridCoGeom" in the doc of Ig2_Sphere_PFacet_ScGridCoGeom [7] ?

I did not make yet the modifications myself because I'm just discovering this part of the code, but, if necessary, I could take care of most of the corrections after minimal discussion here.

Jérôme

[1] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.Law2_ChCylGeom6D_CohFrictPhys_CohesionMoment
[2] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.Law2_CylScGeom_FrictPhys_CundallStrack
[3] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.Law2_ScGeom_FrictPhys_CundallStrack
[4] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.ChCylGeom6D
[5] https://github.com/yade/trunk/blob/e4e757f2e98a620e3177b7a36a1d10f69f6a6a28/pkg/common/Cylinder.cpp#L388
[6] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.Ig2_ChainedCylinder_ChainedCylinder_ScGeom6D
[7] https://yade-dem.org/doc/yade.wrapper.html#yade.wrapper.Ig2_Sphere_PFacet_ScGridCoGeom

Bruno Chareyre (bruno-chareyre) wrote :

Hi Jérôme,
Chained cylinders are considered deprecated, they should be replaced by PFacets and friends - hence removed at some point (better NOT use them). No point improving the doc then.

Besides, one of your comment overlooked the meaning of inheritance in c++ in general and in Yade:
"Do we agree Ig2_ChainedCylinder_ChainedCylinder_ScGeom6D actually return a ChCylGeom6D instance [5] ?
And neither a ScGeom6D (as the name suggests), nor a ScGeom"

Inheritance in c++ must be understood as "B is a A" if B inherits from A. So not only Ig2_ChainedCylinder_ChainedCylinder_ScGeom6D is really returning a ScGeom, *and* also a ScGeom6D (*and* more exactly a ChCylGeom6D); its purpose is really to return a ScGeom[6D] so that older law functors can be used unmodified.
That would not be possible if ChCylGeom6D was not a ScGeom. The doc is thus correct even though it may seem inaccurate.

Bruno

Jérôme Duriez (jduriez) wrote :

Thank you Bruno for comments. I may come back on the naming convention in relation with inheritance later on, in the mean time I fixed 4. at least (not related with inheritance discussion) in https://github.com/yade/trunk/commit/ddb86b587cb500bb5ff141bd802dbb579cca3fb1 and won't touch the rest.

Changed in yade:
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers