-
Notifications
You must be signed in to change notification settings - Fork 18
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
Jiacheng/cdt opt application #837
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #837 +/- ##
=======================================
Coverage ? 76.62%
=======================================
Files ? 463
Lines ? 23035
Branches ? 2580
=======================================
Hits ? 17651
Misses ? 5373
Partials ? 11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments on code structure. But please, do me the favor and add some documentation.
@@ -0,0 +1,425 @@ | |||
#include "cdt_optimization.hpp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for this to be its own component? Should this be used by other components or applications? If not, it is cleaner to keep it directly in the application.
(We also created components for shortest-edge collapse, which actually doesn't make sense. I'll remove that component when I have some spare time)
|
||
namespace wmtk::components { | ||
|
||
std::shared_ptr<Mesh> cdt_optimization( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am really not a fan of having so many input parameters. It would be cleaner if you create a struct instead. An example can be found here.
But even if you stick to the many parameters style, add DOCUMENTATION!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this component is removed now :)
cdt optimization using wildmeshing3d