Skip to content
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

Define 2d model class and Simmetrix GeomSim model loader #96

Open
wants to merge 78 commits into
base: master
Choose a base branch
from

Conversation

cwsmith
Copy link

@cwsmith cwsmith commented Apr 1, 2024

Adds a Model2D class and support for filling the Model2D data structures from a Simmetrix GeomSim model (.smd).

  • tests GeomSim loading on 2d square
  • tests GeomSim loading on a 3d model - expected result is failure (see will_fail_test_func(...) CMake macro)
  • Simmetrix initialization and finalization were moved into the Omega_h::Library constructor.

@cwsmith cwsmith changed the title Define 2d model class and Simmetrix GeomSim loader Define 2d model and mesh class and Simmetrix GeomSim model loader Apr 1, 2024
@cwsmith cwsmith changed the title Define 2d model and mesh class and Simmetrix GeomSim model loader Define 2d model class and Simmetrix GeomSim model loader Apr 1, 2024
@cwsmith
Copy link
Author

cwsmith commented Apr 1, 2024

/runtests

Copy link

github-actions bot commented Apr 1, 2024

Test Result: failure (details)

@cwsmith
Copy link
Author

cwsmith commented Apr 1, 2024

/runtests

Copy link

github-actions bot commented Apr 1, 2024

Test Result: failure (details)

@cwsmith
Copy link
Author

cwsmith commented Apr 2, 2024

/runtests

Copy link

github-actions bot commented Apr 2, 2024

Test Result: failure (details)

@cwsmith
Copy link
Author

cwsmith commented Apr 2, 2024

/runtests

Copy link

github-actions bot commented Apr 2, 2024

Test Result: failure (details)

OMEGA_H_CHECK is a no-op when NDEBUG is defined
@cwsmith
Copy link
Author

cwsmith commented Apr 2, 2024

/runtests

Copy link

github-actions bot commented Apr 2, 2024

Test Result: success (details)

@@ -165,6 +166,7 @@ if(Omega_h_USE_SimModSuite)
set(CMAKE_MODULE_PATH ${OLD_CMAKE_MODULE_PATH})
set(Omega_h_SOURCES ${Omega_h_SOURCES} Omega_h_meshsim.cpp)
set(Omega_h_SOURCES ${Omega_h_SOURCES} Omega_h_matchMeshsim.cpp)
set(Omega_h_SOURCES ${Omega_h_SOURCES} Omega_h_simModel2d.cpp)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably consider using list(APPEND Omega_h_sources ...) (https://cmake.org/cmake/help/latest/command/list.html#append) before we end up with a bunch of calls to set. There are other places that can benefit from this too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Thank you. Draft PR #97 addresses this.

Model2D Mesh2D::updateModel() {
model = Model2D::MeshModel2D_load(*this);
return *model;
Copy link

@matthew-mccall matthew-mccall Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the dereferencing operator work here? It is my understanding that Model2D::MeshModel2D_load returns a Model2D not a pointer to it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Thank you. Fixed with 86e2b4a.

@jacobmerson
Copy link
Collaborator

@cwsmith is this ready to look at? I can take a look over the weekend.

@cwsmith
Copy link
Author

cwsmith commented Apr 5, 2024

@jacobmerson Yes. This is all set. Thank you.

Copy link
Collaborator

@jacobmerson jacobmerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly commented on high level design choices so far.

std::fill(array.data(), array.data()+array.size(), init);
return array;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these constructors can also be used, cant they ?

HostWrite(LO size_in, T offset, T stride, std::string const& name = "");

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HostWrite() = default;
HostWrite(LO size_in, std::string const& name = "");
HostWrite(LO size_in, T offset, T stride, std::string const& name = "");
HostWrite(Write<T> write_in);
HostWrite(std::initializer_list<T> l, std::string const& name = "");

For HostWrite(...), the closest ctor I see is the one that takes an initializer list, but I don't think that will support setting all the values in the array when its length is set at runtime. I also thought there would have been one that initialized all the values but I didn't find it.
Interestingly, there are Write() and Read() ctors that support it:
Write(LO size_in, T value, std::string const& name = "");

Read(LO size, T value, std::string const& name = "");

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwsmith how about if you use stride=0 and set initial value as the offset ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one
HostWrite(LO size_in, T offset, T stride, std::string const& name = "");

Copy link
Author

@cwsmith cwsmith Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good call. I misunderstood what that ctor did. Thanks.

For the record, it creates a HostWrite from a call to the Write(...) ctor with the same signature.

template <typename T>
HostWrite<T>::HostWrite(
LO size_in, T offset, T stride, std::string const& name_in)
: HostWrite<T>(Write<T>(size_in, offset, stride, name_in)) {}

That in turn calls fill_linear(...).
template <typename T>
void fill_linear(Write<T> a, T offset, T stride) {
auto f = OMEGA_H_LAMBDA(LO i) {
a[i] = offset + (stride * static_cast<T>(i));
};
parallel_for(a.size(), f, "Write(size,offset,stride)");
}

I'll create a PR that adds a docstring for that API.

OMEGA_H_CHECK(model.faceToLoopUse == expected_f2lu);
}

int main(int argc, char** argv) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so iiuc, at the moment this functionality added by this PR loads the smd file and queries and stores connectivity and coordinate information in Omega_h::Model2D.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty close. It reads the model topology and some of the geometry from the smd file into the Omega_h::Model2D struct. The arrays in the Model2D struct are currently only accessed for a correctness check for a specific test model.

The goal is to query the Model2D data during adaptation to improve the geometric model approximation. That development is going to take some time and I didn't want to have this code stuck in a branch too long.

@joshia5
Copy link
Collaborator

joshia5 commented May 1, 2024

looks good to me overall, some comments attached

@cwsmith
Copy link
Author

cwsmith commented May 22, 2024

notes from discussion with @jacobmerson

  • this is a 'stub' for future development that will add APIs to access this data during mesh adaptation
  • Model2d is intended to be immutable
    • return Model2d as const
    • make vtxTol and edgeTol as const
    • possibly delete Model2d copy constructor to avoid modification

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

Successfully merging this pull request may close these issues.

4 participants