tests: add test cases for mcarray/mesh verify functions #72
Conversation
|
@xjrc looking good! chipping away at the coverage |
| const std::vector<std::string> CARTESIAN_BASIS = create_basis("x","y","z"); | ||
| const std::vector<std::string> SPHERICAL_BASIS = create_basis("r","theta","phi"); | ||
| const std::vector<std::string> CYLINDRICAL_BASIS = create_basis("r","z"); | ||
|
|
There was a problem hiding this comment.
I know this is only for tests, but we actually use basis as one of the entires for the field protocol, so seeing it here though me off a bit
There was a problem hiding this comment.
Sorry, I didn't notice that conflict! I'll try to think of an alternative name for these collections (space or coordinate system maybe) and update the tests accordingly.
There was a problem hiding this comment.
I think coordinate system is good (the tuples in a coord set are associated with a well known coord system)
| info["dims"])) | ||
|
|
||
| if(!mesh::coordset::uniform::origin::verify(coordset["origin"], | ||
| info["origin"])) | ||
| { |
…verify". Moved some of the mcarray verification tests from the example test case to the verify test case. Added a stub for the "conduit::blueprint::mesh::verify" test cases.
… verify functions.
Fixed a couple of minor bugs in the "blueprint::mesh::coordset::uniform" verify function.
…nate ambiguities.
… verify test cases.
… case. Fixed a couple of bugs in the "blueprint::mesh::topology::unstructured" verify function.
xjrc
force-pushed
the
tests/blueprint_mesh_mcarray_verify
branch
from
75857b1
to
a974f63
Compare
Fixed a number of minor issues in the "blueprint::mesh::index::verify" function.
Fixed a few problems with field verifying in "blueprint::mesh::verify".
|
@cyrush: I believe that this pull request is ready to be merged, but please let me know if you see any problems with the existing test cases and I'll try to revise them as soon as I can. Thanks! |
|
awesome, thanks os much -- I'll take a look soon! |
|
|
||
| // FIXME: Remove the comments from the following line once the verify functions | ||
| // for uniform and rectilinear topologies have been implemented. | ||
| const std::string topology_types[] = {/*"uniform", "rectilinear", */"structured"}; |
There was a problem hiding this comment.
are these functions there, but basically no-ops? If so, can we still wire them up to be called?
There was a problem hiding this comment.
These functions do exist and are currently implement as no-ops, but wiring them up here wouldn't be too helpful. The purpose of this part of the test case is to verify that a valid "explicit" topology isn't accepted as any other form of topology when fed through the blueprint::mesh::topology::verify function (essentially just ensuring that the if clauses in this function point to the correct subroutines). Since the "uniform" and "rectilinear" topology tests are no-ops that return true, calling them here would cause this test to fail (this is actually good because this indicates that these verify functions aren't returning values we'd expect). Ultimately, I think that it's best to keep this commented out for now, especially considering that we already have no-op coverage for the "uniform" and "rectilinear" cases in the "topology_uniform" and "topology_rectilinear" test cases above.
| { | ||
| // FIXME: Does this have to be verified against anything else? What does | ||
| // this basis refer to if it isn't a different path in the mesh structure? | ||
| Node n, info; |
There was a problem hiding this comment.
the basis is an mfem style string for the finite element collection.
I haven't really though about how to verify it, its possible we could enumerate everything mfem supports even without directly using mfem
There was a problem hiding this comment.
Thanks for the clarification on this. I guess it isn't too important that we verify it for now, but I think that keeping the 'fixme' here will be helpful to remind us to expand on this test if/when we decide to expand the blueprint::mesh::field::basis::verify function.
| // FIXME: The coordinate system checking functions shouldn't accept | ||
| // systems such as (y) or (x, z); all successive dimensions should | ||
| // require the existence of previous coordsys dimensions. | ||
| /* |
There was a problem hiding this comment.
we can leave this in commented out -- but I not sure what we want to support.
I can imagine a (complex) case with two 2d meshes that use different major axes, and wanting to plot those both in 3D given some input from a user on how to anchor the unspecified coordinate for each.
There was a problem hiding this comment.
That makes sense to me! I'll just leave this code in until we make a final decision on whether or not these types of coordinate spaces will be supported.
|
this all looks good! Thanks so much for getting this in. For some reason the appveyor (windows) checks didn't run -- I'll resolve anything after I merge. In the future, feel free to create a branch off of the main repo instead of forking. It will make it easier for use to work closer together and make sure all the CI stuff works. |
The changes in this pull request will add test cases to verify the correctness of the
conduit::blueprint::mcarray::verifyandconduit::blueprint::mesh::verifyfunctions in simple cases not covered by the existing example tests (see #55 for more information).