optimad / bitpit

Open source library for scientific HPC
http://optimad.github.io/bitpit/
GNU Lesser General Public License v3.0
117 stars 34 forks source link

Add Levelset RBF generator #352

Closed oguevremont closed 1 year ago

oguevremont commented 1 year ago
kgkara commented 1 year ago

Hello @oguevremont. Thank you very much for your contributions. To be able to merge them on the bitpit master, it will be needed to remove the dependencies on the Eigen library. So, you could use instead the infrastructure provided by bitpit, in the SystemSolver class. After that, we will return with the final review on this pull request.

oguevremont commented 1 year ago

Hello @kgkara. Thank you for the tip. I have removed the dependency to Eigen as suggested and used SystemSolver. The PR is ready for review. I can squish all the commits once the review process is done.

oguevremont commented 1 year ago

Hello @kgkara, thank you for the review and for your time. I have addressed the comments so that the format is more in line with the other examples. The executable RBF_example_00001 can now be run, without arguments (default) or with arguments. The default STL file to be used is cube.stl in the levelset integration tests.

oguevremont commented 1 year ago

@oguevremont Thank you very much for your time. For me this pull request is practically ready, I just have two tiny comments. After these 2 tiny fixes, I will approve this pull request and after the review of @andrea-iob, we can merge it to the master.

Thanks for the comments! I addressed them

kgkara commented 1 year ago

@oguevremont Thank you very much for your time. For me this pull request is practically ready, I just have two tiny comments. After these 2 tiny fixes, I will approve this pull request and after the review of @andrea-iob, we can merge it to the master.

Thanks for the comments! I addressed them

Thank you very much for the changes and your overall contributions.

andrea-iob commented 1 year ago

To speedup the review process I created a new branch with some suggested changes ("ai.RBF.levelset.generator"). If the changes are fine with you you can reset your branch on my updated branch and then we can proceed with the merge.

Here the changes I've made:

oguevremont commented 1 year ago

To speedup the review process I created a new branch with some suggested changes ("ai.RBF.levelset.generator"). If the changes are fine with you you can reset your branch on my updated branch and then we can proceed with the merge.

Here the changes I've made:

  • I've split the changes in self contained commits;
  • support radius is always get through getSupportRadius function (I'm assuming that the optimization of evaluating the inverse of the radius outside the loop is not critical in terms of performances);
  • evaluateBasisPair was moved inside the RBFKernel object and renamed to evalBasisPair for consistency with other methods;
  • the functon getWeights now return a constant reference to the weights;
  • I've fixed some warnings in the example;
  • I've copied the STL needed to run the example in the same directory of the executable;
  • I did some changes to the code style to make it consistent with the existing code.

These changes make sense to me! For future contributions I'll be careful to follow the coding style better, thanks for the input I did the reset of my branch, so you can merge when you'd like