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

Attempt to improve type stability #47

Merged
merged 8 commits into from
Jul 11, 2024
Merged

Attempt to improve type stability #47

merged 8 commits into from
Jul 11, 2024

Conversation

ctessum
Copy link
Contributor

@ctessum ctessum commented Jul 4, 2024

Fixes #38

@mykelk
Copy link
Member

mykelk commented Jul 4, 2024

Thanks for the PR! It looks like some of the tests are not passing?

@ctessum
Copy link
Contributor Author

ctessum commented Jul 4, 2024

I'll try to fix it. I will also add some additional tests.

@ctessum
Copy link
Contributor Author

ctessum commented Jul 5, 2024

OK, the tests are passing now. I also attempted to reduce the allocations in the interpolants function, and marked the remaining allocations with a @test_broken. Let me know what you think.

@mykelk
Copy link
Member

mykelk commented Jul 6, 2024

It seems to break on Julia 1.0. We don't necessarily have to provide backward support to 1.0, but it might be nice if it is not too complicated.

ERROR: LoadError: LoadError: UndefVarError: @allocations not defined
Stacktrace:
 [1] top-level scope
 [2] include at ./boot.jl:317 [inlined]
 [3] include_relative(::Module, ::String) at ./loading.jl:1044
 [4] include(::Module, ::String) at ./sysimg.jl:29
 [5] include(::String) at ./client.jl:392
 [6] top-level scope at none:0
in expression starting at /home/runner/work/GridInterpolations.jl/GridInterpolations.jl/test/runtests.jl:417

@mykelk mykelk requested a review from zsunberg July 6, 2024 06:33
@ctessum
Copy link
Contributor Author

ctessum commented Jul 6, 2024

Sure, I think just removing the test for allocations might fix that. (I don't have julia 1.0 installed to check) Does it work now?

@mykelk
Copy link
Member

mykelk commented Jul 7, 2024

Got it. I think we can merge this after @zsunberg gives us a thumbs up!

Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

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

Thanks @ctessum ! This is a huge help. The only thing I am concerned about is replacing zeros/ones with undef. Can you respond to that before we merge?

Comment on lines +162 to +165
index = MVector{num_points, Int}(undef)
index2 = MVector{num_points, Int}(undef)
weight = MVector{num_points, eltype(x)}(undef)
weight2 = MVector{num_points, eltype(x)}(undef)
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit concerned about this change. How can we be completely sure that it does not change the code logic to initialize these to undef rather than ones or zeros? (note that the tests may pass most of the time even if the code relies on zeros since memory is often initialized to zeros or small numbers.)

I think we should either leave this as is (with ones or zeros) or carefully go through the code below looking at all of the assignments to these variables and write out a brief proof that the initialization does not matter.

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Zeroing memory typically does not have an impact on runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original goal for that change was to reduce allocations, because the ones() and zeros() calls would theoretically be allocating arrays, although I guess the compiler could be factoring that out. From looking at the code I don't believe the arrays need to be explicitly initialized, but just in case I've added it back in, just using broadcasting instead of allocating new arrays for the initialization.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @ctessum ! Yeah, If I remember correctly, we verified that @MVector zeros(...) etc. does not allocate as long as the MVector does not leave the scope of the function. I added an explanatory note.

If we wanted to avoid this and be explicit about proving that the initialization doesn't matter, we could use PushVectors backed with MVectors, but that might require some work.

@mykelk
Copy link
Member

mykelk commented Jul 10, 2024

Cool. I think we can merge once @zsunberg concerns are addressed in this PR.

Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

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

Looks good to me now

@zsunberg zsunberg merged commit 07bfa02 into sisl:master Jul 11, 2024
3 checks passed
@zsunberg
Copy link
Member

zsunberg commented Jul 11, 2024

@ctessum Thanks again!

One note for the future: this PR was a bit hard to read because it included both substantive changes to the code and a bunch of minor style changes (e.g. adding spaces). The style changes are nice, but it is better to make separate PRs, just so that it is easy to see and deal with the important changes 😄

@zsunberg
Copy link
Member

BTW I tried to add allocation tests in #50, but they are failing :/ It seems like we didn't quite prevent all the allocations. The results were the same before this PR though.

@ctessum ctessum deleted the patch-1 branch July 11, 2024 06:03
@ctessum
Copy link
Contributor Author

ctessum commented Jul 11, 2024

Sorry about the style changes, they were just added automatically and I didn't notice until later.

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.

Cannot interpolate complex numbers when passing a 2d array
3 participants