Doc of cylinder-related models

Bug #1746037 reported by Jérôme Duriez
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Yade
Fix Released
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

Revision history for this message
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

Revision history for this message
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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.