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

(feat): add customization API for IntegrationGrid #173

Merged
merged 69 commits into from
May 10, 2023
Merged

Conversation

ilan-gold
Copy link
Collaborator

Description

Focused on customizing the integration domain, this PR allows the user to turn off checks for the domain and also gives a bit more freedom to the grid_func to not have to deal only with a and b in [a,b] but rather just takes is [a,b].

Summary of changes

  • add disable_integration_domain_check argument where applicable
  • change usage of a and b to just integration_domain

How Has This Been Tested?

  • The existing suite of tests should suffice by not breaking since there is not new feature really here.

@github-actions
Copy link

github-actions bot commented Apr 21, 2023

@ilan-gold ilan-gold requested a review from gomezzz April 21, 2023 13:54
@gomezzz
Copy link
Collaborator

gomezzz commented Apr 24, 2023

Description

Focused on customizing the integration domain, this PR allows the user to turn off checks for the domain and also gives a bit more freedom to the grid_func to not have to deal only with a and b in [a,b] but rather just takes is [a,b].

Summary of changes

* add `disable_integration_domain_check` argument where applicable

* change usage of `a` and `b` to just `integration_domain`

How Has This Been Tested?

* The existing suite of tests should suffice by not breaking since there is not new feature really here.

@ilan-gold So, to make sure I remember correctly: This allows integrating multiple integrands on different domains by transforming each integral into [0,1], right?

If so:

  • Does it work for the other integrators (Newton Cotes, MC)? Should it?
  • Can we add a tiny example somewhere how this can be used? I don't know how to test it 🥲
  • Should we maybe change the domain in one of the test cases to use this, otherwise it isn't really tested, is it?

Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

Some smaller comments

torchquad/integration/integration_grid.py Outdated Show resolved Hide resolved
torchquad/integration/utils.py Outdated Show resolved Hide resolved
@ilan-gold ilan-gold requested a review from gomezzz May 8, 2023 09:25
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

only a minor comment, looks good :) 👍

@ilan-gold ilan-gold merged commit ecaebbb into develop May 10, 2023
@ilan-gold ilan-gold deleted the special_grid branch May 10, 2023 13:41
@ilan-gold ilan-gold restored the special_grid branch May 10, 2023 13:41
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.

3 participants