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

1D to Strip #1018

Merged
merged 19 commits into from
Nov 3, 2022
Merged

1D to Strip #1018

merged 19 commits into from
Nov 3, 2022

Conversation

agcapps
Copy link
Member

@agcapps agcapps commented Sep 29, 2022

Adds a verify() function and strip-conversion routines

@agcapps agcapps requested a review from cyrush September 29, 2022 17:03
@agcapps agcapps self-assigned this Sep 29, 2022
@agcapps
Copy link
Member Author

agcapps commented Sep 29, 2022

This is at the discussion stage. In particular, the results need to be tested against other libraries that expect strip meshes, and we need to examine assumptions.

@agcapps
Copy link
Member Author

agcapps commented Sep 29, 2022

Currently, the oneD::verify function expects a structured mesh, as do the conversion routines. verify, at least, should support uniform meshes (specifying a dx and an origin). The conversion routines should support uniform meshes if needed by users. In addition, we should probably also support conversion between 1D structured and 1D uniform and back.

@agcapps agcapps marked this pull request as draft September 29, 2022 17:18
@cyrush
Copy link
Member

cyrush commented Sep 29, 2022

Instead of a new verify case, can we identity existing verified meshes with a subset of properties for this case?

@cyrush
Copy link
Member

cyrush commented Oct 13, 2022

Suggested methods for strip creation:

Suggested top level method:

blueprint::mesh::generate_strip(conduit::Node& mesh,
      const std::string& src_topo_name,
      const std::string& dst_topo_name);

Suggested topology specific method:

blueprint::mesh::topology::generate_strip(const conduit::Node &topo,
       conduit::Node &topo_dest,
       conduit::Node &coords_dest)

@cyrush
Copy link
Member

cyrush commented Oct 13, 2022

Q: For the result strip mesh, does that have to be uniform/rectilinear/structured for the carter use case, or could it be unstructured for any case we could care about?


// Each coordset c in mesh["coordsets"] must have two children of c["values"]
bool consistent_dim = is_dimension_consistent(mesh);
if (!consistent_dim)
Copy link
Member

@cyrush cyrush Oct 13, 2022

Choose a reason for hiding this comment

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

We will be interested in a specific topology, so I thinl we can check the coordset linked with that topology vs all possible coordsets.

Copy link
Member Author

@agcapps agcapps Oct 13, 2022

Choose a reason for hiding this comment

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

That will probably make the code more specific and constrained (in this case, both good things).

res = false;
}

index_t mesh_dim = max_dimension(mesh);
Copy link
Member

Choose a reason for hiding this comment

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

We can use: conduit::blueprint::mesh::utils::coordset::dims for the specific coordset

@agcapps
Copy link
Member Author

agcapps commented Oct 13, 2022

I don't know for sure if Carter wants uniform/rectilinear/structured. I will try to track down a test case and model off that.

Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

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

New methods are on the right track 👍

I a few questions about implementation.

src/libs/blueprint/conduit_blueprint_mesh.cpp Show resolved Hide resolved
// Convert nodal fields
}


Copy link
Member

@cyrush cyrush Oct 25, 2022

Choose a reason for hiding this comment

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

  • remove these before merge

conduit::Node & n = to[key];
const std::string from_name = n.child(0).name();
n.rename_child(from_name, new_name);
n[new_sibling].set(n[new_name]);
Copy link
Member

Choose a reason for hiding this comment

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

Despite the name - I am having a hard time parsing this.

Do we need to pass from & to and the child key?
Why not just pass those nodes like:
copy_node_rename_first_child(src[key],dest[key], new_name, new_sibling)

I also don't understand the context of new_sibling?

I see the rename, but then n[new_sibling] is a copy of the first child after it was renamed?

if (coord_type == "uniform")
{
copy_node_rename_first_child(coordset, dest, "dims", "j", "i");
dest["dims/i"].set(1);
Copy link
Member

Choose a reason for hiding this comment

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

copy_node_rename_first_child(coordset, dest, "dims", "j", "i");

This will copy coordset["dims"] into dest["dims"].
Then rename dest["dims"].child(0) to be named j or dest["dims/j"]
Then copy dest["dims/j"] so it is also dest["dims/i"]
And then you reset:
dest["dims/i"].set(1);

Why have the sibling copy if we are going to reset it in these cases?

Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

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

@agcapps a few more questions. Let me know if you want to meet to discuss.

{
res["coordsets/coords/type"] = "rectilinear";
}

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand this change.

braid_structured aims to create implicit logical topology with on an explicit coordset.

Copy link
Member Author

Choose a reason for hiding this comment

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

woops. I mixed up rectilinear and structured. Just trying to keep the amount of allocation to a minimum.

src/libs/blueprint/conduit_blueprint_mesh.cpp Show resolved Hide resolved
@@ -1101,6 +1101,7 @@ convert_oneD_coordset_to_strip(const conduit::Node &coordset,
else
{
copy_node_rename_first_child(coordset, dest, "values", "y", "x");
dest["values/x"].reset();
Copy link
Member

Choose a reason for hiding this comment

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

reset isn't needed b/c set will take care of it if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Before I added the call to reset(), I was seeing dest["values/x"] hold the same values and dimension as dest["values/y"], except elements 0 and 1 were 0.0 and 1.0. Now much has changed, and after trying it without the call to reset() I see that the expected two values are there.

Removed the call to reset().

@agcapps
Copy link
Member Author

agcapps commented Nov 1, 2022

@cyrush , any more thoughts on this PR?

@cyrush
Copy link
Member

cyrush commented Nov 1, 2022

Yes, I have been pondering.

We can chat at our meeting today.

@agcapps agcapps marked this pull request as ready for review November 2, 2022 15:35
Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

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

Look good,thanks for the iteration on this.

@agcapps agcapps merged commit 74b2fdf into develop Nov 3, 2022
@agcapps agcapps deleted the feature/capps2/1D_strip branch November 3, 2022 18:03
@agcapps agcapps mentioned this pull request Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants