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

BUG: interpolate: fix high memory usage for 2 classes #20404

Merged
merged 7 commits into from
May 22, 2024

Conversation

Matryoskas
Copy link
Contributor

@Matryoskas Matryoskas commented Apr 6, 2024

Reference issue

Closes #20357

What does this implement/fix?

When using the LinearNDInterpolator and CloughTocher2DInterpolator classes, instead of keeping the data about the location and simplicies of all the input points in the memory at once, only keep the data about a single simplex at once, greatly reducing the memory usage with larger inputs

@Matryoskas Matryoskas requested a review from ev-br as a code owner April 6, 2024 23:18
@github-actions github-actions bot added scipy.interpolate Cython Issues with the internal Cython code base defect A clear bug or issue that prevents SciPy from being installed or used as expected labels Apr 6, 2024
@Matryoskas
Copy link
Contributor Author

In MacOS, psutil seems to be returning wildly inconsistent values for the memory used in the test I made: -210 Mb and 141 Mb. For context, in my Linux machine, I always get between 40 and 60 MB. I have decided to increase the tolerance to 512 MB which should still showcase the benefits of this bug fix - before the fix, in my machine, this test would try to allocate an array of size 90 GB.

@ev-br
Copy link
Member

ev-br commented Apr 8, 2024

As a quick note, I believe it's better to add an ASV memory benchmark (https://github.com/scipy/scipy/tree/main/benchmarks) than a unit test.

@Matryoskas
Copy link
Contributor Author

Done. Did not realize that ASV also had memory benchmarks

@Matryoskas Matryoskas force-pushed the fix-memory-usage-bug-in-griddata branch from 207f4bb to 2988926 Compare April 9, 2024 10:59
@Matryoskas Matryoskas force-pushed the fix-memory-usage-bug-in-griddata branch from d1b6ad9 to ec461f1 Compare April 9, 2024 16:06
@Matryoskas
Copy link
Contributor Author

I am getting an error in the Linux - 32 bit check in scipy/_lib/tests/test__util.py:364: in test_basic. I don't think this is an error on my end since this test has no dependencies from my changes. Since the last 2 days of commits to the main branch have skipped the Linux - 32 bit check, is it possible to run this check in the main branch?

@lucascolley
Copy link
Member

Yes, that is showing on other PRs too

@Matryoskas
Copy link
Contributor Author

@ev-br, when you have the chance, can you take a look at my pull request?

@ev-br ev-br added this to the 1.14.0 milestone May 8, 2024
@lucascolley lucascolley changed the title BUG: fix high memory usage in LinearNDInterpolator and CloughTocher2DInterpolator (#20357) BUG: interpolate: fix high memory usage for 2 classes May 17, 2024
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Sorry about the delay @Matryoskas .

This PR looks reasonable. I checked that the main interface with qhull in interpnd.pyx is basically equivalent to what it was before #18376 which caused the memory regression.

I think we should merge this for 1.14 when the CI comes back green.

scipy/interpolate/tests/test_ndgriddata.py Outdated Show resolved Hide resolved
@ev-br ev-br merged commit c106aa5 into scipy:main May 22, 2024
31 checks passed
@ev-br
Copy link
Member

ev-br commented May 22, 2024

... and CI is green, and so is this PR. Thanks @Matryoskas for the patch and @Kai-Striega for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cython Issues with the internal Cython code base defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.interpolate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Memory usage in griddata function in version 1.12
4 participants