Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi all, I just made the merge request: ️ https://gitlab.com/yade-dev/trunk/-/merge_requests/628 --- Gaël Lorieul Research Engineer at Woodtech, Santiago, Chile. PhD holder in CFD from UCLouvain, Belgium. --- ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi all, @Janek and @Bruno, thanks for the instructions, I will push that on Monday ️ Gaël ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi Gaël, On 03/03/2021 00:49, Janek Kozicki (yade) wrote: For example if you have your commits in master, In case you have no idea if the commits are in master, let's step back a bit (else Janek's instructions are already enough): - your modified source code is in a folder "trunk", go to that path in terminal - try "git status", then "git diff". Do you see the changes? - if so "git commit -a -m 'short description of what is commited' " - go back to Janek's email :) Bruno ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi Gaël,
> p₃ = 1 - p₂ - p₃ (1c)
If p₃ was previously uninitialized (or zero). Then this equation must
be incorrect by definition, it uses p₃ on right side.
Ah, but in your photos [1] you wrote p₃ = 1 - p₁ - p₂. :-)
> I also think I might need the confirmation that the fix I propose
> indeed is relevant and safe to be implemented.
The PFacet author should comment on that :)
> Final point: although the changes to be made are small, I would really
> like to submit it personnaly to the git repository so that I learn how
> it is done.
Looks like you already have an account on gitlab: https://gitlab.com/GLorieul
And also you are a member of yade-dev on gitlab:
https://gitlab.com/yade-dev/trunk/-/project_members
This means that you can open a merge request right away. For some
introduction you can have a look at: https://yade-dem.org/doc/gitrepo.html
But in short opening a merge request is very easy: you git push your
stuff as a branch (non-master branch, that is). For example if you
have your commits in master, this should work (but I type from memory
now):
git checkout -b gaelfix # to create ("-b") a branch (gaelfix) for you
git push --set-upstream origin gaelfix # to push it
this command should print (among other messages) a http link to open
a merge request. Just open this link and then in the web browser you
can write some description and then click submit. And that's it :)
Klaus was doing recently the same thing :) I think you two guys could
also open another merge request to update this file gitrepo.html:
https://gitlab.com/yade-dev/trunk/-/blob/master/doc/sphinx/gitrepo.rst
best regards
Janek
PS: nice calculations btw: [1] http://dl.free.fr/mbGj9Moc0 :))
Gaël Lorieul said: (by the date of Tue, 02 Mar 2021 16:31:44 -0300)
> Hello all,
>
> I think I fixed issue 694548.
>
> Contrary to my initial belief, it was not related to what I had
> observed for gridConnection objects (this will be the subject of a
> different thread). Instead, it was a bug regarding the calculation of
> weights. I'll show that in the case of the MWE posted by Jeffrey
> Knowles (issue 694548's original poster).
>
> The motion of PFacet is dictated by that of the 3 gridNode objects it
> connects. Each gridNode recieves a different force value calculated
> from the force of the interaction. The function
> `Law2_ScGridCoGeom_FrictPhys_CundallStrack::go`
> (`pkg/common/Grid.cpp:537`) does that using weights:
> ```
> scene->forces.addForce(geom->id3, geom->weight[0] * -force);
> scene->forces.addForce(geom->id4, geom->weight[1] * -force);
> scene->forces.addForce(geom->id5, geom->weight[2] * -force);
> ```
>
> Those weights are calculated in `pkg/common/PFacet.cpp`
> (`Ig2_Sphere_PFacet_ScGridCoGeom::projection()`) as
>
> ‖v₁‖²(v₀·v₂) - (v₁·v₂)(v₀·v₁)
> p₁ = —(1a)
> ‖v₀‖²‖v₁‖² - (v₀·v₁)²
> ‖v₀‖²(v₁·v₂) - (v₀·v₂)(v₀·v₁)
> p₂ = —(1b)
> ‖v₀‖²‖v₁‖² - (v₀·v₁)²
> p₃ = 1 - p₂ - p₃ (1c)
>
> But actually, it should be:
> ‖v₁‖²(v₀·v₂) - (v₁·v₂)(v₀·v₁)
> p₂ = —(2a)
> ‖v₀‖²‖v₁‖² - (v₀·v₁)²
> ‖v₀‖²(v₁·v₂) - (v₀·v₂)(v₀·v₁)
> p₃ = —(2b)
> ‖v₀‖²‖v₁‖² - (v₀·v₁)²
> p₁ = 1 - p₂ - p₃ (2c)
>
> Hence the bug (MWE "passes" after making the permutation).
>
> In the end, it is only a permutation of p₁,p₂ and p₃. However, let me
> stress that the fix I propose is not just a "hack" (me randomly
> interchanging p1,p2,p3 until I get the result I expected). Instead, I
> did the calculations from scratch again, and the equations (2) are the
> result of that. The scans of my calculations are available here [1]. If
> need be, I can make a LaTeX transcript of those calculations.
>
> However, it uneases me that the consequences of this bug can be seen
> fairly easily, and most certainly the original author of the code would
> have noticed it straight away. Especially since the rest of the
> function seems well written. Hence perhaps things are more complicated
> than that? Could I be breaking something else? To make sure of that, I
> attempted to compile the first version of the code to feature this
> calculation (commit dfec202), but unfortunately without success ️
> (and I don't really want to spend more than the 2.5 days I already
> spent just to try to compile (old) stuff). Note that commit dfec202 is
> the commit that introduced PFacet objects in Yade.
>
> Final point: although the changes to be made are small, I would really
> like to submit it personnaly to the git repository so that I learn how
> it is done. I would need a bit of guidance for that though ️. I also
> think I might need the confirmation that the fix I propose indeed is
> relevant and safe to be implemented.
>
> Gaël
>
> [1]
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hello all, I think I fixed issue 694548. Contrary to my initial belief, it was not related to what I had observed for gridConnection objects (this will be the subject of a different thread). Instead, it was a bug regarding the calculation of weights. I'll show that in the case of the MWE posted by Jeffrey Knowles (issue 694548's original poster). The motion of PFacet is dictated by that of the 3 gridNode objects it connects. Each gridNode recieves a different force value calculated from the force of the interaction. The function `Law2_ScGridCoGeom_FrictPhys_CundallStrack::go` (`pkg/common/Grid.cpp:537`) does that using weights: ``` scene->forces.addForce(geom->id3, geom->weight[0] * -force); scene->forces.addForce(geom->id4, geom->weight[1] * -force); scene->forces.addForce(geom->id5, geom->weight[2] * -force); ``` Those weights are calculated in `pkg/common/PFacet.cpp` (`Ig2_Sphere_PFacet_ScGridCoGeom::projection()`) as ‖v₁‖²(v₀·v₂) - (v₁·v₂)(v₀·v₁) p₁ = —(1a) ‖v₀‖²‖v₁‖² - (v₀·v₁)² ‖v₀‖²(v₁·v₂) - (v₀·v₂)(v₀·v₁) p₂ = —(1b) ‖v₀‖²‖v₁‖² - (v₀·v₁)² p₃ = 1 - p₂ - p₃ (1c) But actually, it should be: ‖v₁‖²(v₀·v₂) - (v₁·v₂)(v₀·v₁) p₂ = —(2a) ‖v₀‖²‖v₁‖² - (v₀·v₁)² ‖v₀‖²(v₁·v₂) - (v₀·v₂)(v₀·v₁) p₃ = —(2b) ‖v₀‖²‖v₁‖² - (v₀·v₁)² p₁ = 1 - p₂ - p₃ (2c) Hence the bug (MWE "passes" after making the permutation). In the end, it is only a permutation of p₁,p₂ and p₃. However, let me stress that the fix I propose is not just a "hack" (me randomly interchanging p1,p2,p3 until I get the result I expected). Instead, I did the calculations from scratch again, and the equations (2) are the result of that. The scans of my calculations are available here [1]. If need be, I can make a LaTeX transcript of those calculations. However, it uneases me that the consequences of this bug can be seen fairly easily, and most certainly the original author of the code would have noticed it straight away. Especially since the rest of the function seems well written. Hence perhaps things are more complicated than that? Could I be breaking something else? To make sure of that, I attempted to compile the first version of the code to feature this calculation (commit dfec202), but unfortunately without success ️ (and I don't really want to spend more than the 2.5 days I already spent just to try to compile (old) stuff). Note that commit dfec202 is the commit that introduced PFacet objects in Yade. Final point: although the changes to be made are small, I would really like to submit it personnaly to the git repository so that I learn how it is done. I would need a bit of guidance for that though ️. I also think I might need the confirmation that the fix I propose indeed is relevant and safe to be implemented. Gaël [1] http://dl.free.fr/mbGj9Moc0 ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hello all, Just a short e-mail to give some news (work is in progress still). I believe that I found the origin of the bug discussed in issue 694548. Not much related to what I suspected initially (hence, this will be the subject of another thread ️). It turns out that the bug was a bit "big" from the start, in the sense that I believe the original writer of the code would have noticed it easily. Hence, it makes me a bit suspicious… Is my "fix" actually safe to use? Or was the code meant to be written this way, and my fix will break something else? Therefore, before sharing my findings I wanted to check if the MWE given in issue 694548 also fails with the first version of the code (dfec202) to feature what I believe is an error in the source. If not, then perhaps my "fix" will be breaking something else… As of now, I am trying to compile Yade as of commit dfec202, which turned out to be quite a nightmare… And that is where I am now still… That being said, I am hopeful to solve it soon. ️ I'll give some more news early next week, hopefully to close this issue. Klaus Thoeni, Wed, 24 Feb 2021 11:36:21 +1100 > I will have to look into this. In the meantime, it would be > excellent if you could share your solution, best is to push > your changes to your fork on gitlab if you have one. As it is, it still needs some cleanup before it can be submitted. That's why I proposed to discuss the general idea first (otherwise, it's not worth the effort). But first, let's close 694548. Since it is a more localized issue, it helps me build confidence in interacting with an existing opensource project. So not a waste of time! ️ Then I'll create a new thread/discussion for the original topic I wanted to discuss. Janek Kozicki, Wed, 24 Feb 2021 12:55:15 +0100 > reformat with the same .clang-format file your old branch (*). Thanks for the tip Janek ️ Bruno Chareyre, Fri, 26 Feb 2021 15:00:53 +0100 > I'm eager to understand which problem you found. :) Thanks for the warm welcome! ️ And I believe I already have an account with the required rights and everything (we communicated last year). But let's check that when the moment comes… Gaël De: Yade-dev en nombre de Bruno Chareyre Enviado: viernes, 26 de febrero de 2021 11:00 Para: yade-dev@lists.launchpad.net Asunto: Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548 Hi Gaël, Thank you for your email. And no you are not colliding with my work since I have it in mind but didn't start anything yet. I would suggest you create a gitlab account if you don't have one so we can add you as yade contributor. You can then push your change to a branch. Something like (assuming you modified the sources directly in local master branch): git push origin master:PFacetFix I'm eager to understand which problem you found. :) Bruno On 23/02/2021 21:59, Gael Lorieul wrote: Hi ️ It has been a long time… I have come accross thread #694548 on the "Yade-users" mailing list (from January 2021), which concerned unphysical collisions between PFacets (the dynamics of the rebound does not correspond to what one's physical/common sense would expect). I wanted to share that I encountered a very similar issue but with gridConnection objects (I work with cylinders), and that I found a solution. The latter should be easily extendible both to PFacet/PFacet and PFacet/gridConnection collisions (although I only tested gridConnection/gridConnection collisions). I am not sure how elegant or computationally efficient it is, but it has given me good results on the (very few) simulations I have tested it on. I initially wanted to test my solution on full-scale simulations before submiting it to the Yade code base, so as to present a code that is robust and mature. But as it has been almost a full year since then (the company I work for gave me assignments other than DEM), I feel that perhaps I should reach out for other ways to share this information… This implies that although I am quite confident of my fix, I am not 100% certain either… Perhaps a first step would be for me to present what I have found, and how my fix works? I am not sure as to where to present it: This mailing list? Thread #694548? You tell me ️ Gaël PS: I have started to re-implement my solution in yade e07e530 (26 Nov 2020) (my previous implementation was based on yade 1b4ae97 (8 Feb 2020)). Hopefully the work I do on e07e530 will not be difficult to port to the latest yade versions… (It seems the yade project started using clang-format (or similar tool) sometime between e07e530 and 1b4ae97 so `diff` outputs half the file: not very helpful… ️) PS2: Could thread #695558 be also concerned by this same uncorrect physical behavior? PS3: I hope that I am not colliding with Bruno's work on the subject (ac
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi Gaël, Thank you for your email. And no you are not colliding with my work since I have it in mind but didn't start anything yet. I would suggest you create a gitlab account if you don't have one so we can add you as yade contributor. You can then push your change to a branch. Something like (assuming you modified the sources directly in local master branch): /git push origin master:PFacetFix / I'm eager to understand which problem you found. :) Bruno On 23/02/2021 21:59, Gael Lorieul wrote: Hi ️ It has been a long time… I have come accross thread #694548 on the "Yade-users" mailing list (from January 2021), which concerned unphysical collisions between PFacets (the dynamics of the rebound does not correspond to what one's physical/common sense would expect). I wanted to share that I encountered a very similar issue but with gridConnection objects (I work with cylinders), and that I found a solution. The latter should be easily extendible both to PFacet/PFacet and PFacet/gridConnection collisions (although I only tested gridConnection/gridConnection collisions). I am not sure how elegant or computationally efficient it is, but it has given me good results on the (very few) simulations I have tested it on. I initially wanted to test my solution on full-scale simulations before submiting it to the Yade code base, so as to present a code that is robust and mature. But as it has been almost a full year since then (the company I work for gave me assignments other than DEM), I feel that perhaps I should reach out for other ways to share this information… This implies that although I am quite confident of my fix, I am not 100% certain either… Perhaps a first step would be for me to present what I have found, and how my fix works? I am not sure as to where to present it: This mailing list? Thread #694548? You tell me ️ Gaël PS: I have started to re-implement my solution in yade e07e530 (26 Nov 2020) (my previous implementation was based on yade 1b4ae97 (8 Feb 2020)). Hopefully the work I do on e07e530 will not be difficult to port to the latest yade versions… (It seems the yade project started using clang-format (or similar tool) sometime between e07e530 and 1b4ae97 so `diff` outputs half the file: not very helpful… ️) PS2: Could thread #695558 be also concerned by this same uncorrect physical behavior? PS3: I hope that I am not colliding with Bruno's work on the subject (according to Bruno's e-mail of Tue, 12 Jan 2021 07:50:51) ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
> (It seems the yade project started using clang-format (or similar > tool) sometime between e07e530 and 1b4ae97 so `diff` outputs half > the file: not very helpful… ️) Hi, reformat with the same .clang-format file your old branch (*). Then you will be back to meaningful diffs. I used this trick more than once when fighting old bugs which appeared long before clang-format and comparing with newer versions: this eliminates all formatting differences and only meaningful code changes remain. best regards Janek (*) you can use: `scripts/clang-formatter.sh ./` (copied to old branch, along with .clang-format config file) Gael Lorieul said: (by the date of Tue, 23 Feb 2021 20:59:33 +) > Hi ️ > > It has been a long time… > > I have come accross thread #694548 on the "Yade-users" mailing list (from > January 2021), which concerned unphysical collisions between PFacets (the > dynamics of the rebound does not correspond to what one's physical/common > sense would expect). > > I wanted to share that I encountered a very similar issue but with > gridConnection objects (I work with cylinders), and that I found a solution. > The latter should be easily extendible both to PFacet/PFacet and > PFacet/gridConnection collisions (although I only tested > gridConnection/gridConnection collisions). I am not sure how elegant or > computationally efficient it is, but it has given me good results on the > (very few) simulations I have tested it on. > > I initially wanted to test my solution on full-scale simulations before > submiting it to the Yade code base, so as to present a code that is robust > and mature. But as it has been almost a full year since then (the company I > work for gave me assignments other than DEM), I feel that perhaps I should > reach out for other ways to share this information… > > This implies that although I am quite confident of my fix, I am not 100% > certain either… > > Perhaps a first step would be for me to present what I have found, and how my > fix works? I am not sure as to where to present it: This mailing list? Thread > #694548? You tell me ️ > > Gaël > > > PS: I have started to re-implement my solution in yade e07e530 (26 Nov 2020) > (my previous implementation was based on yade 1b4ae97 (8 Feb 2020)). > Hopefully the work I do on e07e530 will not be difficult to port to the > latest yade versions… (It seems the yade project started using clang-format > (or similar tool) sometime between e07e530 and 1b4ae97 so `diff` outputs half > the file: not very helpful… ️) > > PS2: Could thread #695558 be also concerned by this same uncorrect physical > behavior? > > PS3: I hope that I am not colliding with Bruno's work on the subject > (according to Bruno's e-mail of Tue, 12 Jan 2021 07:50:51) > -- -- Janek Kozicki, PhD. DSc. Arch. Assoc. Prof. Gdańsk University of Technology Faculty of Applied Physics and Mathematics Department of Theoretical Physics and Quantum Information -- http://yade-dem.org/ http://pg.edu.pl/jkozicki (click English flag on top right) ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
Re: [Yade-dev] A suggestion/contribution regarding bug of Yade-users #694548
Hi Gael, thanks for your email. Somehow question #694548 slipped through the cracks so I didn't follow the discussion. Indeed, there is a problem on how forces are distributed. I will have to look into this. In the meantime, it would be excellent if you could share your solution, best is to push your changes to your fork on gitlab if you have one. Then send me the link and I will have a look. Or you could send me the files you changed via email. Thanks, Klaus On Wed, Feb 24, 2021 at 7:59 AM Gael Lorieul wrote: > Hi ️ > > It has been a long time… > > I have come accross thread #694548 on the "Yade-users" mailing list (from > January 2021), which concerned unphysical collisions between PFacets (the > dynamics of the rebound does not correspond to what one's physical/common > sense would expect). > > I wanted to share that I encountered a very similar issue but with > gridConnection objects (I work with cylinders), and that I found a > solution. The latter should be easily extendible both to PFacet/PFacet and > PFacet/gridConnection collisions (although I only tested > gridConnection/gridConnection collisions). I am not sure how elegant or > computationally efficient it is, but it has given me good results on the > (very few) simulations I have tested it on. > > I initially wanted to test my solution on full-scale simulations before > submiting it to the Yade code base, so as to present a code that is robust > and mature. But as it has been almost a full year since then (the company I > work for gave me assignments other than DEM), I feel that perhaps I should > reach out for other ways to share this information… > > This implies that although I am quite confident of my fix, I am not 100% > certain either… > > Perhaps a first step would be for me to present what I have found, and how > my fix works? I am not sure as to where to present it: This mailing list? > Thread #694548? You tell me ️ > > Gaël > > > PS: I have started to re-implement my solution in yade e07e530 (26 Nov > 2020) (my previous implementation was based on yade 1b4ae97 (8 Feb 2020)). > Hopefully the work I do on e07e530 will not be difficult to port to the > latest yade versions… (It seems the yade project started using clang-format > (or similar tool) sometime between e07e530 and 1b4ae97 so `diff` outputs > half the file: not very helpful… ️) > > PS2: Could thread #695558 be also concerned by this same uncorrect > physical behavior? > > PS3: I hope that I am not colliding with Bruno's work on the subject > (according to Bruno's e-mail of Tue, 12 Jan 2021 07:50:51) > > ___ > Mailing list: https://launchpad.net/~yade-dev > Post to : yade-dev@lists.launchpad.net > Unsubscribe : https://launchpad.net/~yade-dev > More help : https://help.launchpad.net/ListHelp > ___ Mailing list: https://launchpad.net/~yade-dev Post to : yade-dev@lists.launchpad.net Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
