-
Notifications
You must be signed in to change notification settings - Fork 64
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
enh: add shell namespaces and verify for mesh bp #55
Conversation
added implementation of mesh/coordset verify added implementation of mesh verify that relies on: mesh/coordset (implemented) mesh/topology (unimplemented) mesh/field (unimplemented) added info output to mcarray verify wired generic interface for path based protocol identification ex: blueprint::verify(n,"mesh/coordset") maps to blueprint::mesh::coordset::verify(n,"mesh/coordset") still evolving structure of the info output for verify, but it looks like the following: { valid = "true" or "false" (if errors) errors = ["error string 1", "error string 2"] child { valid = "true" or "false" (if errors) errors = ["error string 1", "error string 2"] } ... } this allows us to report errors at any part in a hierarchy, but assumes protocols won't require the names "valid" or "errors"
bool res = true; | ||
|
||
// mcarray needs to be an object or a list | ||
if( ! (n.dtype().is_object() || n.dtype().is_list()) ) |
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 an incredibly minor complaint, but I feel like this line doesn't match the coding standards used for of the rest of Conduit. In particular, it doesn't feel like the expanded spacing in this statement fits; something like if(!(n.dtype().is_object() || n.dtype().is_list()))
seems more appropriate.
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.
sounds good
} | ||
|
||
// required: "topologies", with at least one child | ||
// each child must conform to "mesh::topology" |
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.
The functionality below for verifying the topologies
path is nearly identical to that used above to verify the coordsets
path. The commonalities between these two segments should be factored out if possible.
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.
thats a good observation, if we can condense these checks into reusable functions that sounds like a good improvment
} | ||
} | ||
|
||
// optional: "fields", each child must conform to "mesh::field" |
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.
The functionality used below to verify the fields
path is also nearly identical to the coordsets
path verification functionality and should be considered during factoring.
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.
It may be worth noting in the info
node that this section is optional as is done with the extra dimensions for nodes in the coordset::verify_coordset_uniform
function below.
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.
yes, we should add an info entry to optional indicating that fields exist.
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.
Given how much duplicated/specifically factored code we need in order to implement these verification functions, I think that it may be better to tackle this implementation by leveraging rapidjson
instead. We can use rapidjson
to perform a lot of the basic schema validation (see here) and then just implement some of our more advanced requirements on top of this. This approach has a number of benefits, the most important being that (1) it tremendously reduces the amount of code we need to write and (2) it makes it easier to see/modify the mesh schema structure (see some of Spack's schemas for examples).
Does this alternate method make sense and/or seem feasible? If it doesn't make sense, please let me know and I'll try to lay out what I'm thinking more clearly.
info["errors"].append().set(msg); | ||
res = false; | ||
} | ||
else if(!dims["i"].dtype().is_number()) |
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.
All of these dimensional value verification procedures have very similar code as well and should be factored if possible.
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.
One of my goals is for the the code used to verify the protocol to reflect how the data will be consumed.
Given this, I don't think we should use rapidjson here b/c that would require transforming the data (or its schema) into json. At this point the data is not json -- it's in conduit's in memory format.
Even with an exemplar schema, or access to standard set of schema validation primitives I suspect we will still have to have some custom code for cases where schema validation doesn't get us all the way there. In that case we have two styles of validation which will be harder to maintain.
We could try something similar in conduit (compare to exemplar nodes), but like schema validation I don't think that will cover all of what is admissible.
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.
You certainly bring up a lot of valid points, but I still think that leveraging rapidjson
here will help us a lot more than it will hinder us. If you don't mind, could we discuss this further during our meeting tomorrow afternoon? If you're not convinced after I try to elaborate more on why I think this is a good idea, I won't pursue this any further; I just want to advocate a bit more for this idea because I think it could save us a lot of work.
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.
sure, feel free to explore it more. I don't know if we will have too much time to get into the details tomorrow, but if you have a prototype of how it would look that would help frame the discussion.
One quick addendum to my previous comment: using the schema validation functionality in |
index gen commit will resolve #38 |
also: refactored verify logging logic + helpers added check for grid_function to bp::mesh::generate_index
@xjrc -- this is coming along. The last piece is mesh:topology::verify. I may take a pass at it, but I will still need lots of help with testing. Also wondering if how I am doing the nested namespaces in the .cpp file is confusing.
instead of
Thoughts? |
@cyrush: Even though the first version is more verbose, I like it better because I can easily just read the function title and know exactly which I'll try to take a look at this sometime later this week. I'm pretty busy preparing for my presentation in a few weeks, but I'm going to try to be done by Friday. After I'm finished, I'll start contributing some test cases. |
thanks, I agree -- I'll make changes to the how namespaces are used in the cpp file. |
remove none info variants of verify funcs
refactor bp verify sub cases into actual protocols
added more structure to coord_system protocol to support cart, cylnd, and spherical
Changes Unknown when pulling d213787 on enh/2016_09_blueprint_mesh_verify into * on master*. |
…m/llnl/conduit into enh/2016_09_blueprint_mesh_verify
Changes Unknown when pulling 15b5685 on enh/2016_09_blueprint_mesh_verify into * on master*. |
Changes Unknown when pulling 6b0897b on enh/2016_09_blueprint_mesh_verify into * on master*. |
@xjrc this should include all of the additions for mesh bp verify (#40) and index gen ( #38 ) #49 will require a good amount of changes to the new verify code -- I am wondering if we should merge this PR, and start a new branch / PR focused on unit tests to improve coverage of the new code (and any fixes we find) (https://coveralls.io/builds/8274435/source?filename=src%2Flibs%2Fblueprint%2Fblueprint_mesh.cpp). That way I can easily move in parallel to resolve #49. What do you think? |
@cyrush: I agree; I think that merging this branch will make it much easier to move forward with testing. I'm going to start writing some tests today, but I'll hold off on pushing them to a public branch until this is merged to make future merges simpler. |
@xjrc merged this pr, feel free to create a new branch and open a new pr |
bool | ||
check_cyln_coord_sys_axis_name(std::string &name) | ||
{ | ||
return ( name == "r" || name == "z"); |
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.
@cyrush: Shouldn't cylindrical coordinates be expressed using 3 coordinates (i.e. r, z, and theta)? If we're only planning to use cylindrical to describe cylindrical profiles then there isn't an issue, but I think that the third coordinate will be necessary for fully describing 3D geometries.
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.
rz is for 2D cylindrical, the "r" component collapses 2 comps from 3d.
all the sim codes I know about only support 2D-cylindrical -- for the 3D case the r would be broken out into two components.
added implementation of mesh/coordset verify
added implementation of mesh verify that relies on:
mesh/coordset (implemented)
mesh/topology (unimplemented)
mesh/field (unimplemented)
added info output to mcarray verify
wired generic interface for path based protocol identification
ex:
blueprint::verify(n,"mesh/coordset")
maps to
blueprint::mesh::coordset::verify(n,"mesh/coordset")
still evolving structure of the info output for verify, but
it looks like the following:
{
valid = "true" or "false"
(if errors)
errors = ["error string 1", "error string 2"]
child
{
valid = "true" or "false"
(if errors)
errors = ["error string 1", "error string 2"]
}
...
}
this allows us to report errors at any part in a hierarchy, but
assumes protocols won't require the names "valid" or "errors"