Skip to content

(feat): add JIT integrator tests#166

Merged
ilan-gold merged 23 commits into
developfrom
jit_tests
Mar 8, 2023
Merged

(feat): add JIT integrator tests#166
ilan-gold merged 23 commits into
developfrom
jit_tests

Conversation

@ilan-gold
Copy link
Copy Markdown
Collaborator

@ilan-gold ilan-gold commented Mar 6, 2023

Description

Adds tests for JIT integration

Resolved Issues

How Has This Been Tested?

  • Added 1d JIT tests

Related Pull Requests

  • #PR

@ilan-gold
Copy link
Copy Markdown
Collaborator Author

ilan-gold commented Mar 6, 2023

@gomezzz I've added some tests for this that work. There are a few catches. The main thing is that we don't test repeated integration for the same integrand-N-dim JIT combination (which is the point of JIT, but maybe not something we actually need to test). This is because the test suite doesn't really track integrand_shape in the right places besides whether or not the integrand is multi-dimensional in general (from #160). So we have a few options here:

  1. Refactor the test suite to allow for finer grained control over what is run and how (i.e., more explicitly tracking integrand_shape) - so a different PR in addition to this one. We'll then be able to say something like "only run shape (2,2,) integrands repeatedly" or the like
  2. Add another step to this PR to allow for re-running the 1d tests with a single JIT call (the functionality for which already exists), as opposed to the current state of this PR where JIT happens every time.
  3. Keep what we have (i.e., JIT every integrand).

@ilan-gold
Copy link
Copy Markdown
Collaborator Author

P.S One advantage of refactoring would be that we don't have to run all the tests. I'm not sure what the goal of "unit testing" JIT is besides functionality i.e., I'm not sure we actually need to check accuracy of every single integral. We're more interested in not-crashing.

@gomezzz
Copy link
Copy Markdown
Collaborator

gomezzz commented Mar 6, 2023

P.S One advantage of refactoring would be that we don't have to run all the tests. I'm not sure what the goal of "unit testing" JIT is besides functionality i.e., I'm not sure we actually need to check accuracy of every single integral. We're more interested in not-crashing.

Agree :)

So we have a few options here:

Any of them are okay for me. At the moment the tests take ~4min which is fine and I'll take any improvement :D

@ilan-gold
Copy link
Copy Markdown
Collaborator Author

@gomezzz I think I found a pretty clean way of doing this actually, so going to run with that.

@ilan-gold ilan-gold requested a review from gomezzz March 7, 2023 13:18
Copy link
Copy Markdown
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.

few minor comments

Comment thread torchquad/tests/boole_test.py
Comment thread torchquad/tests/boole_test.py Outdated
Comment on lines +98 to +104
def integrate(*args, **kwargs):
nonlocal jit_integrate
if jit_integrate is None:
jit_integrate = bl.get_jit_compiled_integrate(
dim=1, N=N, backend=backend
)
return jit_integrate(*args, **kwargs)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is the redeclaration necessary? was declared just above? 🤔

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same two comments apply to some of the other files as well, I think ✌️

Comment thread torchquad/tests/simpson_test.py
Comment thread torchquad/tests/trapezoid_test.py
@ilan-gold
Copy link
Copy Markdown
Collaborator Author

Well that now-failing test is flaky - fails sometimes, passes others.

@gomezzz
Copy link
Copy Markdown
Collaborator

gomezzz commented Mar 8, 2023

Well that now-failing test is flaky - fails sometimes, passes others.

let's increase the bounds on this one? 🤔

@ilan-gold
Copy link
Copy Markdown
Collaborator Author

Sounds good to me

@ilan-gold ilan-gold requested a review from gomezzz March 8, 2023 10:33
Copy link
Copy Markdown
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.

👍

@ilan-gold ilan-gold changed the base branch from fix_jit to develop March 8, 2023 12:55
@ilan-gold ilan-gold merged commit f820796 into develop Mar 8, 2023
@ilan-gold ilan-gold deleted the jit_tests branch March 8, 2023 13:04
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