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

Record buffer length needed for face data #624

Closed
wants to merge 1 commit into from

Conversation

agcapps
Copy link
Member

@agcapps agcapps commented Oct 10, 2020

The blueprint mesh representation uses a helper class, TopologyMetadata, to find and record the relations from element to faces and nodes. There was a test in that computation that was too restrictive, resulting in a segfault in test t_blueprint_mesh_verify when it was compiled Debug on Windows by either MSVC or Intel 19 compilers.

@agcapps agcapps requested a review from xjrc October 10, 2020 17:27
@agcapps agcapps marked this pull request as draft October 10, 2020 17:27
@agcapps agcapps requested a review from cyrush October 10, 2020 17:28
@agcapps agcapps linked an issue Oct 10, 2020 that may be closed by this pull request
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.966% when pulling ae0da82 on bugfix/agcapps/fixTopoMetadata into edc3f72 on master.

@agcapps
Copy link
Member Author

agcapps commented Oct 11, 2020

@xjrc , this change removes the check whether dim_shape.embedding == NULL.

Why do is_poly(), is_polyhedral(), is_polygonal() check whether embedding == NULL? Put differently, what does foo.is_poly*() -> true mean about foo?

@xjrc
Copy link
Member

xjrc commented Oct 12, 2020

Why do is_poly(), is_polyhedral(), is_polygonal() check whether embedding == NULL?

If a 2+-D entity doesn't have an embedding, that implies that the entity's sub-entity composition isn't statically defined, which is only the case for polytopal entities in Conduit. This ordering cannot be statically defined for such entities because the number of sub-entities is variable, and thus any sub-entity creation code must necessarily be implicit/generative rather than static. For example, the implicit/generative rules for polygons is that each pair of adjacent indices (including at the boundary of the arrays, so 0 and n-1) refer to vertices that comprise a single edge of a particular polygon.

Put differently, what does foo.is_poly*() -> true mean about foo?

Here is what each of the is_poly* functions mean:

  • is_poly(): The topology is polytopal, which means it consists of either polygons (2D) or polyhedra (3D).
  • is_polyhedral(): The topology consists of polyhedrea (3D).
  • is_polygonal(): The topology consists of polygons (2D).

@agcapps
Copy link
Member Author

agcapps commented Oct 14, 2020

@xjrc , @cyrush , I don't know why the Main Docker_ubuntu build failed. Could you get it to retry please?

@cyrush
Copy link
Member

cyrush commented Oct 16, 2020

@agcapps with @bmhan12's changes in #632, this should be resolved, so I am closing this PR.

Can you double check on windows, and if there are more issues we can attack with a new PR?

@cyrush cyrush closed this Oct 16, 2020
@agcapps
Copy link
Member Author

agcapps commented Oct 17, 2020

Yes, @cyrush , this is superseded by @bmhan12 's changes in #632 which fix the problem on Windows.

@agcapps agcapps deleted the bugfix/agcapps/fixTopoMetadata branch October 17, 2020 17:53
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.

t_blueprint_mesh_generate crashes on Windows
4 participants