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

Add pyramid and wedge support #1023

Merged

Conversation

JustinPrivitera
Copy link
Collaborator

@JustinPrivitera JustinPrivitera commented Oct 7, 2022

Resolves #132

Adds support in the mesh blueprint for pyramids and wedges.
Adds versions of braid that are comprised of pyramids and wedges.

Once merged I'll put up a PR in VisIt for the bp plugin.

The following still need to be done:

  • Add to docs - crop pictures for basic wedges and pyramids + add code includes for docs
  • fix failing CI
  • add wedges and pyramids to everywhere that the other shape types are used that I haven't already

Comment on lines +192 to +210
//---------------------------------------------------------------------------//
inline float64 braid_init_example_point_scalar_field_calc_dx(index_t npts_x)
{
return (float) (4.0 * PI_VALUE) / float64(npts_x - 1);
}


//---------------------------------------------------------------------------//
inline float64 braid_init_example_point_scalar_field_calc_dy(index_t npts_y)
{
return (float) (2.0 * PI_VALUE) / float64(npts_y-1);
}


//---------------------------------------------------------------------------//
inline float64 braid_init_example_point_scalar_field_calc_dz(index_t npts_z)
{
return (float) (3.0 * PI_VALUE) / float64(npts_z-1);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do we feel about these? I did this instead of hard coding the calculation wherever it was needed.

Copy link
Member

Choose a reason for hiding this comment

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

The most general way would be to eval at a specific x,y,z instead of the dx,dy,dz.
Should be an easy refactor, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why did I do this? If I remember correctly, it was because I didn't want to recalculate dx, dy, and dz for each x,y,z I am using to calculate the field val. There is a function (below, probably?) that calculates the field val for a given x,y,z and dx,dy,dz. Does that make sense?

@JustinPrivitera JustinPrivitera marked this pull request as ready for review October 7, 2022 02:25
@JustinPrivitera JustinPrivitera requested review from agcapps and removed request for cyrush and agcapps October 7, 2022 17:53
@JustinPrivitera JustinPrivitera marked this pull request as draft October 7, 2022 18:25
@cyrush
Copy link
Member

cyrush commented Oct 13, 2022

Suggestion I don't want to forget: Please add a Changelog entry.

@cyrush
Copy link
Member

cyrush commented Oct 17, 2022

Angle of the pictures looks good!

I would crop them down a bit so they fill more space on the docs page.

https://llnl-conduit--1023.org.readthedocs.build/en/1023/blueprint_mesh.html#pyramids

(compare pyramids with the hexs visual)

We also need the yaml output of the bluperint tree for wedges and pyramids.

And there is a small typo with the wedges entry ref in the table:

image

@@ -932,6 +932,10 @@ supported values for this parameter and their corresponding effects are outlined
+--------------------------------+--------------------+-------------------+-------------------+------------------+
| `hexs <Hexs_>`_ | 3d | explicit | explicit | hex |
Copy link
Member

Choose a reason for hiding this comment

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

Missing an _ to make ref

@JustinPrivitera
Copy link
Collaborator Author

JustinPrivitera commented Oct 17, 2022

Angle of the pictures looks good!

I would crop them down a bit so they fill more space on the docs page.

https://llnl-conduit--1023.org.readthedocs.build/en/1023/blueprint_mesh.html#pyramids

(compare pyramids with the hexs visual)

We also need the yaml output of the bluperint tree for wedges and pyramids.

And there is a small typo with the wedges entry ref in the table:

image

Hi. I left this in draft b/c I knew I still needed to fix up these last few things w/ the docs. Nice catch on the wedges entry ref in the table. I'll get to these very very soon 😄

I was hoping to get these last few things done before you reviewed, my bad.

@cyrush
Copy link
Member

cyrush commented Oct 17, 2022

@JustinPrivitera yes, no worries, I had some time to review this morning so I wanted to see how things were going :-)

@JustinPrivitera JustinPrivitera marked this pull request as ready for review October 17, 2022 18:47
@agcapps
Copy link
Member

agcapps commented Oct 17, 2022

Thanks, @JustinPrivitera !

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.

Looks good, thanks for working though all of this.

@JustinPrivitera
Copy link
Collaborator Author

Looks good, thanks for working though all of this.

I'll merge once I figure out the strangeness with silo... the silo version of wedges looks very different from the blueprint version.

@JustinPrivitera JustinPrivitera merged commit 7e1c363 into develop Oct 20, 2022
@JustinPrivitera JustinPrivitera deleted the task/JustinPrivitera/09_30_22/pyramid_wedge_support branch October 20, 2022 02:27
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.

mesh bp: add support for pyramid and wedge elements
3 participants