-
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
Major clenup #465
Major clenup #465
Conversation
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'll do the full review as soon as the tests are running again.
Codecov Report
@@ Coverage Diff @@
## main #465 +/- ##
==========================================
+ Coverage 83.35% 85.07% +1.71%
==========================================
Files 215 209 -6
Lines 5954 5788 -166
==========================================
- Hits 4963 4924 -39
+ Misses 991 864 -127
Flags with carried forward coverage won't be shown. Click here to find out more.
|
src/wmtk/io/MshReader.hpp
Outdated
class MshReader : public MeshReader | ||
{ | ||
protected: | ||
void read_aux(const std::filesystem::path& filename, Mesh& mesh) override; |
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 think you're still in the middle of writing things but it'd be good to have comments on what aux/cb are in this class
@@ -55,7 +55,7 @@ class EdgeCollapse : public TriMeshOperation, private TupleOperation | |||
|
|||
private: | |||
Tuple m_output_tuple; | |||
const OperationSettings<EdgeCollapse>& m_settings; | |||
// const OperationSettings<EdgeCollapse>& m_settings; |
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.
why remove the settings? i think there is a future refactor to handle how we pass these settings around but for now this practice is the "way" we're doing things?
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.
they were unused :)
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.
Please open up an issue for that but do not just comment out stuff.
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.
There are several things that we need to discuss. Let's find some time soon.
This PR should have been at least 5 seperate PRs. Let's merge this asap and create follow-up issues. That's also the reason why I will only comment the PR and not request changes.
mesh.initialize(F); | ||
|
||
mesh_utils::set_matrix_attribute(V, "position", PrimitiveType::Vertex, mesh); | ||
auto mesh = MeshReader::read(options.file); |
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.
That won't do the job but we have issue already that reminds me that this needs to be fixed.
components/wmtk_components/isotropic_remeshing/isotropic_remeshing.cpp
Outdated
Show resolved
Hide resolved
components/wmtk_components/regular_space/internal/RegularSpace.hpp
Outdated
Show resolved
Hide resolved
@@ -55,7 +55,7 @@ class EdgeCollapse : public TriMeshOperation, private TupleOperation | |||
|
|||
private: | |||
Tuple m_output_tuple; | |||
const OperationSettings<EdgeCollapse>& m_settings; | |||
// const OperationSettings<EdgeCollapse>& m_settings; |
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.
Please open up an issue for that but do not just comment out stuff.
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
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
No description provided.