Skip to content

Paper review #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ralfHielscher opened this issue Jun 30, 2020 · 5 comments
Closed

Paper review #5

ralfHielscher opened this issue Jun 30, 2020 · 5 comments

Comments

@ralfHielscher
Copy link
Contributor

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.

  1. 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
Copy link
Owner

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 d4fb03b.

  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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

ralfHielscher commented Dec 2, 2020 via email

@DorianDepriester
Copy link
Owner

DorianDepriester commented Dec 2, 2020

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 🧐

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants