8000 Refactor ngrid by marco-2023 · Pull Request #268 · theochem/grid · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactor ngrid #268

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 12 commits into from
Apr 23, 2025
Merged

Refactor ngrid #268

merged 12 commits into from
Apr 23, 2025

Conversation

marco-2023
Copy link
Collaborator

Refactored the ngrid module to improve readability and add features.

  • Added weights property which returns the weights for each argument combination (due to its combinatorial nature returns a generator)
  • Added points property which returns the combinations of arguments (due to its combinatorial nature returns a generator). Each point is a tuple with several arrays, each array corresponds to an argument of the multigrid function to integrate.
  • Added support for non-vectorized functions. These will be evaluated point by point during the integration.

@PaulWAyers I think this PR improved the previous class. Nonetheless, I still have some doubts:

  1. The current class used is a derived class of Grid because of this, several methods are inherited moments, save and get_localgrid. I think that save and get_localgrid can be implemented (I have some doubts about the meaning of get_localgrid in this context ), but I don't see how to get moments.
  2. This one is related to my previous doubt. Wouldn't be better to make a new class called perhaps MultiDomainGridIntegrator that is only responsible for integrating functions with more than one argument?

@PaulWAyers
Copy link
Member
PaulWAyers commented Feb 14, 2025 •
edited
Loading

Thanks @marco-2023 . I'm going to add @FarnazH to the review panel for this one. I'm also curious if @Ali-Tehrani has any comments/thoughts.

@PaulWAyers PaulWAyers requested a review from FarnazH February 14, 2025 23:30
@PaulWAyers
Copy link
Member

I talked to @marco-2023 about this today. The main issue is that the basegrid defines moments and local grids. Doing local grid is "easy" for ngrid (Marco says it's ~30 minutes of work.)

I looked a little bit at the code. Some comments:

  1. The moments seem tied to 3-dimensional integration. I don't think it is worth trying to define n-dimensional spherical harmonics. (They exist, but it seems more painful than it is worth, but it is feasible.) This would make it possible to define "all the types of moments". I do not know a use case for this, however.

  2. I think the easiest solution might be to support only Cartesian moments. The order of a Cartesian moment in ngrid is just the sum of the orders of the Cartesian moments of the composing low-dimensional grids.

What do you think @Ali-Tehrani and @FarnazH ?

@Ali-Tehrani
Copy link
Collaborator
Ali-Tehrani commented Feb 25, 2025 •
edited
Loading

  • I would recommend writing out what the items (and their dimension) of the generators are from points() and weights() function, because I'm kinda confused. I'm guessing its (x_1, x_2, x_3, \cdots, x_N) as a list[list], where $x_i$ is a $n_i$-dimensional vector and N is the number of domains.

    • In addition, I would recommend writing out f: \mathbb{R}^{n_1} \times \cdots \times \mathbb{R}^{n_N} \rightarrow \mathbb{R}. I'm pretty certain most if not all, fields understand this notation and makes it clear for me at-least.

  • If you're going to do a high-dimensional moments function, then centers should be centers = List[List[Points]] (C, N, n_i) where C is the number of centers.

    • Since there isn't any use-cases of any of the spherical harmonics moments, raise a NotImplementedError(This has not been implemented, please make an Issue on the Grid page and we'll get back to you.) or something. Probably good to comment that Spherical Harmonics github page and this pull-request there as well.

    • Radial moments
      $$\int_{ \mathbb{R}^{n_1} \times \cdot \times \mathbb{R}^{n_N} } || \mathbf{r} - R_c ||^{k} f(\mathbf{r})$$,
      the norm is based on product space of vector-spaces and so the norm is $\sqrt{\sum_{i=1}^N ||x_i||_{\mathbb{R}^{n_i}}^2 }$, where $\mathbf{r} = (x_1, \cdots, x_N)\in \mathbb{R}^{n_1} \times \cdot \times \mathbb{R}^{n_N}$ and I'm guessing $k = 0, \cdots, M$ with $M = n_1 + \cdots + n_N$.

      • I think the code must be easy, Concatenate all the centers together to get shape (C, N * n_i), then feed that into moment function in BaseGrid class super().moments(orders, centers, func_vals, "radial") . I could be wrong though because of self.weights, but the code should be identical so you can copy-paste and use MultiDomainGrid.integrate.

    • Cartesian moments, similarly to above this, if you set it up correctly, it should work by just feeding it into the Base class super().moments(). But you'll have to change the moments so that orders: int, list so the user can arbitrary put in the orders as a list. The code would break when type_mom = (pure, pure-radial) but should work as a list for cartesian and radial. Or you can copy-paste, the code should work in N-dimensions.

      • Generation of the Cartesian orders for high-dimensions might be too much (you'll have to generate Cartesian orders per $\mathbb{R}^{n_i}$), and would probably be better to let the user specify exactly what order they want

I am not guaranteeing if these suggestions would make it easy or not. I think you'll have to change self.weights as well within BaseGrid.moments, or copying the BaseGrid.moments into MultiDomainGrid and modify it.
My personal preference is to just raise a NonImplementedError, since no-one has a good use case and tell the user to make a issue. I'm also indifferent as to whether this should be a sub-class of grid or not, it doesn't hurt to be a sub-class even though it overrides everything.

PaulWAyers
PaulWAyers approved these changes Apr 7, 2025
View reviewed changes
Copy link
Member
@PaulWAyers PaulWAyers left a comment

Choose a reason for hiding this comment

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

LGTM

@marco-2023 marco-2023 merged commit 6c8dea4 into theochem:master Apr 23, 2025
8 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulWAyers PaulWAyers PaulWAyers approved these changes

@FarnazH FarnazH Awaiting requested review from FarnazH

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@marco-2023 @PaulWAyers @Ali-Tehrani
0