-
Notifications
You must be signed in to change notification settings - Fork 40
Disable allocation logging if reshape allocates #217
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a mechanism to disable allocation logging when certain operations (like reshape
) unexpectedly allocate, mitigating JuliaLang issue #58634. It adds a global flag to control allocation checks, updates the solver and tests to respect this flag, and adds a CI helper to auto-disable checks when reshape allocates.
- Introduce
check_allocs
flag and control functions inVoronoiFVM.jl
- Extend solver’s allocation warning to honor
check_allocs
- Add CI logic in tests to detect reshape allocations and disable checks
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
test/test_inplacelu.jl | Switched allocation macros and wrapped tests in check_allocs guard |
test/alltests.jl | New helper functions to detect reshape allocations and disable checks |
src/vfvm_solver.jl | Updated allocation warning condition to include check_allocs() |
src/VoronoiFVM.jl | Added _check_allocs global and check_allocs /check_allocs! APIs |
docs/src/internal.md | Documented the new check_allocs APIs |
Comments suppressed due to low confidence (6)
test/alltests.jl:6
- [nitpick] The function name
treshape
is ambiguous. Consider renaming totest_reshape
ormeasure_reshape_alloc
for clarity.
function treshape(X::AbstractArray, n, m)
test/alltests.jl:12
- [nitpick] The name
talloc
is not descriptive. You might usemeasure_allocations
ortest_allocations
to convey its purpose.
function talloc(; n = 10, m = 20)
test/test_inplacelu.jl:167
- The macro
@allocations
is not defined in Base; this will error. Replace it with@allocated
or import/define@allocations
if you need a custom macro.
m1 = @allocations inplacelu_piv_stridearray(10, Float64)
src/VoronoiFVM.jl:68
- There's a typo in the docstring:
alllocations
should beallocations
(one fewer "l").
Should alllocations be checked ?
test/alltests.jl:3
- Importing the module name via
using VoronoiFVM: VoronoiFVM
is invalid. You can eitherusing VoronoiFVM
or explicitly import onlycheck_allocs!
.
using VoronoiFVM: VoronoiFVM, check_allocs!
test/alltests.jl:20
- [nitpick] Consider capitalizing "Julia" in the warning message for consistency:
"Disabling allocation checks due to Julia issue #58634"
.
@warn "Disabling allocation checks due to julia issue #58634"
For more explanations: The problem is reporte in JuliaLang/julia#58634 and seems to improve with recent nightly. |
Ready to merge... |
try to mitigate JuliaLang/julia#58634