-
Notifications
You must be signed in to change notification settings - Fork 17
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
Hackathon/edge mesh #378
Hackathon/edge mesh #378
Conversation
fixed compilation problems |
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 should probably be a PR draft. I started leaving some comments and then realized this is mostly a copy-paste of PointMesh :)
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.
Looks all good so far. I did not go through the functions labeled as not tested yet. I guess the next step is to add tests and make sure all these functions do what they are supposed to do.
I uploaded the current code and thought the main skeleton was done, but still, it should have some problems that need to be fixed. |
…wildmeshing-toolkit into hackathon/edge_mesh
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 is a massive PR. My main concern is that there is a lot of code of which I am not sure if it was tested. I added a few comments regarding that. Especially, if you copy code, throw an exception in every function. The fact that they look like they do something reasonable but might have not been tested is extremely dangerous. So please make sure that every function is at least called once in a test or throws an exception.
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.
Looks very good already. I am just still a bit concerned that there are functions that were implemented but never tested.
DEBUG_EdgeMesh::operator==
needs to be fixed. There are a few more minor comments. I think the PR is almost there.
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.
lgtm
src/wmtk/EdgeMesh.cpp
Outdated
@@ -81,8 +81,9 @@ Tuple EdgeMesh::switch_tuple(const Tuple& tuple, PrimitiveType type) const | |||
|
|||
// TODO: This is for special case self-loop, just to make sure the local vid of the returned |
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 this TODO still a TODO?
Codecov Report
@@ Coverage Diff @@
## main #378 +/- ##
==========================================
+ Coverage 81.95% 82.43% +0.48%
==========================================
Files 154 158 +4
Lines 4162 4470 +308
==========================================
+ Hits 3411 3685 +274
- Misses 751 785 +34
Flags with carried forward coverage won't be shown. Click here to find out more.
|
What is left before we can merge this? I'll trust daniel's review on this, but what's left to finish off his comments? |
I think daniel's comments are finished. There was one merge main branch to our branch after finishing the comments. I think nothing else there? |
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.
Just fix that compilation issue and then let's finish this PR.
Just add dummy .hpp/.cpp and modified cmake files.