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 VTKm Probe filter for regular grid resampling #1247

Merged
merged 42 commits into from
May 8, 2024

Conversation

nicolemarsaglia
Copy link
Contributor

No description provided.

//move origin to lower left corner
m_origin[0] = m_origin[0] - (m_dims[0]/2);
m_origin[1] = m_origin[1] - (m_dims[1]/2);
m_origin[2] = m_origin[2] - (m_dims[2]/2);
Copy link
Member

Choose a reason for hiding this comment

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

I want to make sure I understand the origin shift.
Does VTK-m create Uniform Datasets from the center of the Volume?

Copy link
Contributor Author

@nicolemarsaglia nicolemarsaglia Feb 1, 2024

Choose a reason for hiding this comment

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

VTKm creates uniform datasets from the lower left corner, whereas conduit starts in the middle (and I'm copying that format), so I shifted middle to lower left. Does that make sense?

https://vtk-m.readthedocs.io/en/latest/dataset.html#creating-uniform-grids

Copy link
Member

Choose a reason for hiding this comment

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

In general, Conduit also starts with an origin + dx,dy,dz. Which is lower left corner with respect to an overall coordinated system.

That said the blueprint example you are using may be centering things at 0 and have volume that covers both negative and positive parts of the overall coordinate system.

Want to make sure we aren't confusing a specific case for testing with what we need for the general case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh ok, that's good to know! Must have just saw this special case and assumed it was the default. Let me add some tests with different input data.

const int num_domains = this->m_input->GetNumberOfDomains();

for(int i = 0; i < num_domains; ++i)
{
Copy link
Member

Choose a reason for hiding this comment

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

I think we need more logic here to support the distributed memory case.

If all ranks are doing the resample to the same grid, we would have do a reduction to compose the results on each task.

Also, we may want to distribute the output grid across MPI tasks as well for high res resampling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have to compose the results? If each rank is responsible for it's own portion of the data, they will either have a point that gets sampled or they won't, and in the end they are still in charge of their own portion of the new grid.

I have a concern about needing to do a large resample, right now we have to create the global grid on each rank.

Copy link
Contributor Author

@nicolemarsaglia nicolemarsaglia Feb 1, 2024

Choose a reason for hiding this comment

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

I will add a parallel test

Copy link
Member

Choose a reason for hiding this comment

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

yes - I think so. Let's chat about the cases high bandwidth.

To test this you can use the 2D spiral mulit-domain blueprint example and dish out domains to different mpi tasks.

@nicolemarsaglia
Copy link
Contributor Author

Will require updated VTK-m. (2.2 or 2.1.1)

///
///. (We know that all ranks will have something to reduce, b/c
///. even if they have no domains, the still created the local
/// output grid)
Copy link
Member

Choose a reason for hiding this comment

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

@nicolemarsaglia see note here of how we can accomplish this w/o MergeDataset -- allowing us to use vtk-m 2.1
Also, this would be more memory efficient b/c merging will create a fused unstructured grid which can require a lot more space if the input was uniform (explicit coords and connectivity, maybe copying and repacking of field arrays)

@cyrush
Copy link
Member

cyrush commented May 2, 2024

@nicolemarsaglia this looks good to me, we can discuss today!

@cyrush cyrush added this to the 0.9.3 milestone May 2, 2024
@nicolemarsaglia nicolemarsaglia merged commit 109e128 into develop May 8, 2024
20 checks passed
@nicolemarsaglia nicolemarsaglia deleted the task/2024_01_vtkm_grid_sampling_filter branch May 8, 2024 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants