search

DorianDepriester / MTEX2Gmsh

Matlab toolbox for generating 2D meshes from EBSD data
https://doriandepriester.github.io/MTEX2Gmsh/
MIT License
41 stars 18 forks source link

Paper review #5

Closed ralfHielscher closed 4 years ago

ralfHielscher commented 4 years ago

Hi Dorian,

I went through the code and found it very useful and impressive. I think there are some places for improvement. I will list them here but I don't think that they are showstopper for the review process.

  1. A central point seems to be the reordering of the boundary segments . Here you use an algorithm based on Euler Tours. MTEX itself contains already some Euler Tour algorithms in mtex/tools/graph_tools which might be more efficient. Another idea can be found in the last paragraph of grain2d/subSet. Here the boundary segments are ordered without the use of any Euler Tours command by making use of the fact that grain2d.poly contains already a ordered list of vertices.

  2. For me it would make a lot of sense to include the ordering directly into MTEX. Currently, the grainboundary segments are not ordered at all (unless there is only a single grains). The ordering you suggest seems very reasonable and could easily be the default ordering in MTEX.

  3. The spline interpolation seems to be used only for plotting. This is not so clear from the documentation. Also, wouldn't it make more sense to use some buildin spline function like spline instead of a custom one.

  4. Smoothing the boundary while keeping triple points and points at the outer boundary fixed should actually be an option within MTEX :) Same is true for the simplification of the geometry,

  5. When publishing the help files it is useful to set

    setMTEXpref('generatingHelpMode',true);

    as it avoids some output artifacts.

  6. I'm not completely convinced about having an extra class for the grains. Wouldn't it make more sense to inherit from grain2d? When I understood this correctly the main difference to the class grain2d is that the boundary segments are sorted and the gmshGeo keeps track of this order. I'm correct?

DorianDepriester commented 4 years ago

Thank you @ralfHielscher for your report. Here are the answers to the points you have raised:

  1. I am aware that my own EulerPath is not very efficient (very slow compared to the MTEX's EulerCycles). Still, under some rare circumstances, the EulerCycles() fails to walk down the path as I wish. See for instance file attached: because of the self 'touching' grain boundary, the path is ambiguous. This situation generally occurs because of outliers, so it often can be avoided by cleaning the data (e.g. w/o 1-dot-size grains); but sometimes outlier removal do not fix this.

  2. Considering the point above, if the EulerCycles roughly complies with my needs in a future version of MTEX, I would be glad to use it instead of my DIY EulerPath.

  3. The Bspline approximation is indeed only used in MATLAB for plotting. But it is called in Gmsh before meshing (as a built-in feature). As a result, the implementation of the Bspline function is required here to ensure that the geometry plotted in MATLAB is consistent with that read by Gmsh before meshing.

  4. I completely agree with that point. Also note that higher-order vertex can be reached (quadruple points and so on), but they are not listed in the grain2d property triplePoints.

  5. I have added setMTEXpref('generatingHelpMode',true); at the beginning of each file in /doc. See commit d4fb03b628f784d7ce0f2e5a8aeb5b5c93c58cba.

  6. I do agree that subclassing grain2d could be clearer, and this could be done in a future release. Actually, the data in the gmshGeo are stored in a very different way; so, for somehow historical reasons, the gmshGeo class has been designed from scratch.

ralfHielscher commented 4 years ago

Hi Dorian,

thank you for your responses. I agree with you in all points. Especially when it comes to the sorting of the boundary segments this is something I still need to do in MTEX.

Congratulations to this piece of software.

Ralf.

DorianDepriester commented 3 years ago

Hi @ralfHielscher , Just to mention that I have finally managed to use your EulerCycles() function for sorting the paths, with some hacks so that the resulting paths fits with my needs.

ralfHielscher commented 3 years ago

Hi Dorian,

that is great. Actually, I would like to sort the boundaries in grain2d right from the beginning as it makes plotting much easier. Do you see a chance to include this into the constructor of grain2d? Also it should not take too much time :)

A second point: Would you like to participate in the MTEX workshop next year? I would be happy to listen to a presentation from your side.

Ralf.

Ralf Hielscher Tel: +371-531-38556 Fakultät für Mathematik +371-531-22200 (Sekr.) Technische Universität Chemnitz Fax: +371-531-22109 Reichenhainer Str. 39 E-mail: ralf.hielscher@mathematik.tu-chemnitz.de D-09126 Chemnitz http://www.tu-chemnitz.de/~rahi

On Wed, Dec 2, 2020 at 10:35 AM Dorian Depriester notifications@github.com wrote:

Hi @ralfHielscher https://github.com/ralfHielscher , Just to mention that I have finally managed to use your EulerCycles() function for sorting the paths, with some hacks so that the resulting paths fits with my needs.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DorianDepriester/MTEX2Gmsh/issues/5#issuecomment-737110424, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACBYJ2PUULWENOI5ZSFL2ITSSYC6DANCNFSM4OMWJQDA .

DorianDepriester commented 3 years ago

Hi Ralf, It would be a great idea to implement such a sorting into the grain2d constructor. I'm not sure my 'hack' is very neat (I'm not familiar with graph theory), so maybe you should have a look on it before :monocle_face:

It would be a great honor to take part of the MTEX workshop. Of course I would be pleased to participate!