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

Refactor ngrid #268

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Refactor ngrid #268

wants to merge 10 commits into from

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

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 ?

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.

2 participants