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

Unbounded poisson solve 3d #135

Merged
merged 6 commits into from
Dec 15, 2022
Merged

Unbounded poisson solve 3d #135

merged 6 commits into from
Dec 15, 2022

Conversation

fankiat
Copy link
Owner

@fankiat fankiat commented Dec 12, 2022

Fixes #133 , merge after #132 .

@bhosale2 in this PR the MPIDomainDoublingCommunicator3D (currently named) is actually generic for both 2d and 3d in its current form now. In other words, that same communicator can be used for UnboundedPoissonSolverMPI2D. However, the communicator includes a few abstractions, which may or may not be straightforward, but I've placed some comments on some of the steps used. Nonetheless, there are a few things I need a second opinion on.

  1. Since the domain doubling communicator is now a generic one, should we put this communicator elsewhere? If we do, where do you think is the best place, given that we have mpi_utils.py, mpi_utils_2d.py, and mpi_utils_3d.py?
  2. Following up on that, depending on what we do, maybe it's a good time to revisit Refactor MPI utils #91 — making a dimension-agnostic mpi_utils.py?

The above points of course only make sense (somewhat) to discuss further if we decide to actually move the domain doubling communicator to a more appropriate place. If we keep the domain doubling communicators separated in 2d and 3d, we can probably keep the code in this PR as is, and keep the communicator in the 2d poisson solver as a simpler code for reference (where the communicator is somewhat hardcoded and assumes only slab decomposition). Finally, depending on what we do here, I will come up with some test cases for the communicator, which I believe works correctly, given the poisson solver gives the right results.

@fankiat fankiat added enhancement New feature or request prio:high High priority labels Dec 12, 2022
@fankiat fankiat self-assigned this Dec 12, 2022
@bhosale2
Copy link
Collaborator

@fankiat I feel it should not be called utils anymore, since its an important sub-module of the library. Maybe you can think of moving these MPI parts to a sub-module under sopht-mpi called mpi_comm or a better name? This should happen in a separate PR ideally.

@bhosale2 bhosale2 self-requested a review December 15, 2022 18:26
@fankiat
Copy link
Owner Author

fankiat commented Dec 15, 2022

@fankiat I feel it should not be called utils anymore, since its an important sub-module of the library. Maybe you can think of moving these MPI parts to a sub-module under sopht-mpi called mpi_comm or a better name? This should happen in a separate PR ideally.

Hmm.. That's one way to go about it I guess. Ok then, we can look into this in a separate PR after this PR is merged. Maybe we open an issue for the refactoring and I can take care of that issue accordingly. Thanks!

Copy link
Collaborator

@bhosale2 bhosale2 left a comment

Choose a reason for hiding this comment

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

minor.

Copy link
Collaborator

@bhosale2 bhosale2 left a comment

Choose a reason for hiding this comment

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

LGTM. @fankiat after this make sure you open an issue called Road to Graduation or so, and start listing the functionalities for scaling, running high res, global profiling, etc. to prioritize accordingly.

@bhosale2 bhosale2 merged commit c1674e3 into main Dec 15, 2022
@bhosale2 bhosale2 deleted the 133_unbounded_poisson_solve_3d branch December 15, 2022 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prio:high High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unbounded poisson solve 3d
2 participants