add option to also transform matset_values to silo #685

cyrush · 2021-01-26T17:39:26Z

No description provided.

cyrush · 2021-01-26T17:40:29Z

I am testing this out in visit, I may add some more example fields to venn if it helps me verify all the moving pieces.

coveralls · 2021-01-26T18:15:43Z

Coverage increased (+0.09%) to 86.317% when pulling 46223f8 on task/2021_01_matset_vals_to_silo into b3d1ba0 on develop.

…x bugs with relay mesh bp truncate write

cyrush · 2021-01-29T19:35:06Z

@xjrc I think this is ready for further review, can you take a pass?

Once this is ready, I plan todo a 0.7.0 release for use in VisIt. (I am still testing this in parallel in VisIt)

cyrush · 2021-01-29T19:36:27Z

also, this should resolve #676 (but we may need more docs)

xjrc

Looks good, Cyrus. I'm requesting changes with my review because I believe that we need to update the docs and tests to indicate that material_map can be included with any form of matset entry now, but is optional for multi-buffer material sets. Thanks for the changes and let me know if you'd like any help with synthesizing or reviewing the docs!

xjrc · 2021-01-29T19:47:50Z

src/libs/blueprint/conduit_blueprint_mesh.cpp

                while(mats_itr.has_next())
                {
                    mats_itr.next();
-                    idx_matset["materials"][mats_itr.name()];
+                    idx_matset["material_map"][mats_itr.name()] = mat_id;


Unless I'm missing something, this could be simplified by using mats_itr.index() instead.

FYI: Good suggestion, I made this change.

xjrc · 2021-01-29T19:48:34Z

src/libs/blueprint/conduit_blueprint_mesh.cpp

            }
            else if(matset.has_child("materials"))
            {
+                // NOTE: I believe path is deprecated ... 


This is correct; we either have and use the material_map provided, or we use a default ordering to create a temporary material_map for processing. Thus, this branch can be safely removed.

xjrc · 2021-01-29T20:01:35Z

src/tests/blueprint/t_blueprint_mesh_verify.cpp

+            CHECK_MESH(verify_matset_index,mindex,info,true);
+            mindex["material_map/mat2"].set(2);
+            CHECK_MESH(verify_matset_index,mindex,info,true);
+        }


Great! However, for the sake of completeness, we also need this to be tested at the matset level for multi-buffer matsets, and for the documentation to be updated.

xjrc · 2021-01-29T20:05:08Z

src/libs/blueprint/conduit_blueprint_mesh_matset_xforms.cpp

            temp.set_external(DataType::int64(1), &mat_index);
-            temp.to_data_type(int_dtype.id(), matset_mat_map[mat_name]);
+            temp.to_data_type(int_dtype.id(), matset_mat_map[curr_mat_name]);


I think this look could also be simplified with vf_itr.index().

FYI: Good suggestion, I made this change.

xjrc · 2021-01-29T20:16:13Z

also, this should resolve #676 (but we may need more docs)

Just saw this; yes, I agree with your assessment based on my read-through of the code.

cyrush · 2021-01-29T23:42:53Z

@xjrc thanks for the suggestions and for keeping me honest.

I added more docs, strengthened the verify checks and tests.

Let me know if think they are more ways to improve.

rhornung67 · 2021-01-29T23:44:20Z

@cyrush more docs, more tests (less problems :-) Is that like tastes great, less filling?

cyrush · 2021-01-30T03:11:54Z

@rhornung67 hmm, less problems is good thing - but I am not sure filling is a bad thing. Especially because #686 would be a good thing.

xjrc

I just have one more comment on the way matset::verify currently works, but overall this looks great and could be merged as-is. Thanks for all of you hard work on these features, Cyrus!

xjrc · 2021-02-01T19:48:52Z

src/libs/blueprint/conduit_blueprint_mesh.cpp

-            // verifies that it's one level deep and that each child child houses
-            // an integer-style array.
-            vfs_res &= verify_object_field(protocol, matset, info, "material_map");
+            vfs_res &= verify_matset_material_map(protocol,matset,info);


I think I'd make this check separate, similar to what's done in the multi-buffer case; this way, the user can check for a valid material_set entry (i.e. an object with all integer values) without depending on a valid volume_fractions entry.

I agree, I shifted the logic to outside

xjrc · 2021-02-01T19:52:30Z

src/libs/blueprint/conduit_blueprint_mesh.cpp

+            const std::vector<std::string> &map_mats = matset["material_map"].child_names();
+            const std::set<std::string> vf_matset(vf_mats.begin(), vf_mats.end());
+            const std::set<std::string> map_matset(map_mats.begin(), map_mats.end());
+            if(vf_matset != map_matset)


I think this should probably check that vf_matset is a subset of map_matset instead of forcing them to be equal. It's a slim and out-there use case, but I could imagine a user having one material_map spread across all matset entries, and in that case it may have extra material values that aren't relevant to some particular entries.

done, also added a test case

xjrc · 2021-02-01T19:58:39Z

src/tests/blueprint/t_blueprint_mesh_verify.cpp

@@ -1127,6 +1235,34 @@ TEST(conduit_blueprint_mesh_verify, matset_general)
                n["element_ids"].set(DataType::int32(5));
                CHECK_MESH(verify_matset,n,info,true);
                CHECK_MATSET(n,is_uni_buffer,is_material_dominant);
+
+                //--------------------------------------//
+                // check for optional use of material_map


check for non-optional use of material map

xjrc · 2021-02-01T19:59:10Z

src/tests/blueprint/t_blueprint_mesh_verify.cpp

+                CHECK_MESH(verify_matset,n,info,false);
+
+
+                n.remove("material_map");


Thanks for being so thorough with these tests, Cyrus!

xjrc

Looks great, @cyrush; thanks!

cyrush added 2 commits January 26, 2021 09:35

add option to also transform matset_values to silo

f54bbf0

typo

406b588

cyrush added this to the 0.7.0 milestone Jan 26, 2021

cyrush requested a review from xjrc January 26, 2021 17:39

cyrush added 2 commits January 29, 2021 10:32

add venn mat_check field, change matset index to use material_map, fi…

e98bc6d

…x bugs with relay mesh bp truncate write

odds and ends to finish to_silos

5fe5845

xjrc requested changes Jan 29, 2021

View reviewed changes

more docs, more tests (less problems :-)

ba073cb

xjrc approved these changes Feb 1, 2021

View reviewed changes

more tweaks to verify for mat maps

46223f8

xjrc approved these changes Feb 4, 2021

View reviewed changes

cyrush merged commit 57c39c9 into develop Feb 4, 2021

cyrush deleted the task/2021_01_matset_vals_to_silo branch February 4, 2021 23:35

cyrush mentioned this pull request Feb 5, 2021

make material numbering more explicit #676

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add option to also transform matset_values to silo #685

add option to also transform matset_values to silo #685

cyrush commented Jan 26, 2021

cyrush commented Jan 26, 2021

coveralls commented Jan 26, 2021 •

edited

Loading

cyrush commented Jan 29, 2021

cyrush commented Jan 29, 2021

xjrc left a comment

xjrc Jan 29, 2021

cyrush Jan 29, 2021

xjrc Jan 29, 2021

xjrc Jan 29, 2021

xjrc Jan 29, 2021

cyrush Jan 29, 2021

xjrc commented Jan 29, 2021

cyrush commented Jan 29, 2021

rhornung67 commented Jan 29, 2021

cyrush commented Jan 30, 2021

xjrc left a comment

xjrc Feb 1, 2021

cyrush Feb 4, 2021

xjrc Feb 1, 2021

cyrush Feb 4, 2021

xjrc Feb 1, 2021

xjrc Feb 1, 2021

xjrc left a comment

		CHECK_MESH(verify_matset,n,info,false);


		n.remove("material_map");

add option to also transform matset_values to silo #685

add option to also transform matset_values to silo #685

Conversation

cyrush commented Jan 26, 2021

cyrush commented Jan 26, 2021

coveralls commented Jan 26, 2021 • edited Loading

cyrush commented Jan 29, 2021

cyrush commented Jan 29, 2021

xjrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjrc commented Jan 29, 2021

cyrush commented Jan 29, 2021

rhornung67 commented Jan 29, 2021

cyrush commented Jan 30, 2021

xjrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjrc left a comment

Choose a reason for hiding this comment

coveralls commented Jan 26, 2021 •

edited

Loading