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

Typed instance-free allocation #192

Merged
merged 13 commits into from
Jul 28, 2024
Merged

Typed instance-free allocation #192

merged 13 commits into from
Jul 28, 2024

Conversation

mateuszbaran
Copy link
Member

Addresses JuliaManifolds/Manopt.jl#400 . I think similar is a better default because rand is doing much more work than required in these circumstances.

allocate_as definitely need many special cases which I will work on later.

@kellertuer what do you think about this approach?

@mateuszbaran mateuszbaran added the enhancement New feature or request label Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.90%. Comparing base (5fb1add) to head (5dfb16c).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #192      +/-   ##
==========================================
- Coverage   99.93%   99.90%   -0.03%     
==========================================
  Files          30       31       +1     
  Lines        3264     3292      +28     
==========================================
+ Hits         3262     3289      +27     
- Misses          2        3       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellertuer
Copy link
Member

I will have to check this carefully;
I think the point we meant was when a state (in Manopt.jl) is parametrised by point (so we need the type of a field p::P) , but maybe only partially initialised so we do not yet need the actual point.

Then knowing P would be enough.

My idea for a solution is for now something like

  • having 2 keywords for p and P.
  • if p is provided / allocated, P can be inferred as its type
  • if P is provided, p should not be allocated

Will check whether your function does that.

@kellertuer
Copy link
Member

So I looked at this a bit closer, and I still have to figure out more places, here is an example meant in the original issue: in

https://github.com/JuliaManifolds/Manopt.jl/blob/4a52a82122b3054bb4369c3e7e391bb0cd8d197d/ext/ManoptLRUCacheExt.jl#L40C5-L40C6

a p (and an X) is allocated just to get and use its type afterwards. For these cases it would be nice to avoid the allocation.
I do not yet fully understand, what allocate_as does, can you maybe extend its documentation a bit?

@mateuszbaran
Copy link
Member Author

allocate_as doesn't solve all issues you have there. It primarily enables replacing rand(M) with allocate_as(M) when you don't actually need a random point but just some memory for future use (plus it's a bit more flexible than rand). If you just need the type, you can use typeof(allocate_as(M)). It's not an optimal solution but it can't be done better automatically from what we currently have, so it's either that or defining a new function with lots of methods just for this purpose.

@kellertuer
Copy link
Member

That's neat indeed, I currently used rand(M) quite often for the allocate_as you refer to (similarly zero_vector(M,p) for vectors, but that might even be okay I think).

What do you mean bu “not optimal” – in sense of speed/allocation or in sense of usability? We could map point_type(M) = typeof(allocate_as(M)) to have something nicely readable?

@mateuszbaran
Copy link
Member Author

(similarly zero_vector(M,p) for vectors, but that might even be okay I think).

zero_vector should be fine, but you can also use allocate_as(M, TangentSpaceType()) to allocate a tangent vector.

What do you mean bu “not optimal” – in sense of speed/allocation or in sense of usability? We could map point_type(M) = typeof(allocate_as(M)) to have something nicely readable?

Yes, I meant in the sense of speed and allocations. Julia currently can't remove the actual allocation in allocate_as even though we only need the type. There was some work in Julia compiler that could lead to this type of optimization (it was called "escape analysis") but they hit some roadblock and it hasn't been finished.

We could map point_type(M) = typeof(allocate_as(M)) to have something nicely readable?

Sure, that makes sense. Maybe we could name it default_type so that it can also handle tangent vectors? Or do you have another idea for naming a function that returns type of tangent vectors?

@kellertuer
Copy link
Member

default_type sounds also ok, just that both allocate_as(M) and default_type(M) both read a bit like you would allocate a manifold or ak for the default type of the manifold (and not for points thereon); but sure on the other hand, good naming is complicated for sure. I will think a bit about a name – the general idea here (hough not related to the original issue as we noticed) is nice.

@mateuszbaran
Copy link
Member Author

I've added default_type, so only the hardest part remains: naming 😅 .

@kellertuer
Copy link
Member

Yes, naming is the only thing left. What I do not like is that allocate_as(M) reads like it would allocate a manifold.

Here are some ideas

  • allocate_on(M) since points live on a manifold, even more one could do allocate_on(M,p) for tangents? Though using TangentSpace(M,p) is probably better.
  • allocate_point(M) also sounds ok

@mateuszbaran
Copy link
Member Author

  • allocate_on(M) since points live on a manifold, even more one could do allocate_on(M,p) for tangents? Though using TangentSpace(M,p) is probably better.

allocate_on is a good idea. The problem with tangents is that it would be good to have a way to allocate them without an explicit instance of p, that's why I've proposed this interface.

@kellertuer
Copy link
Member

Just knowing the type of the point that should be doable, sure; it is then not necessarily a is_vector valid one, but points allocated here are neither. But I think we would need the point type maybe?

@mateuszbaran
Copy link
Member Author

I think we could do allocate_on(M, TangentSpaceType(), PointType) for vector types based on point types. ManifoldsBase.jl doesn't have any types to define that for but it could be useful in Manifolds.jl.

@kellertuer
Copy link
Member

That sounds reasonable. In the long run (among others for a forthcoming solver) I would love to have Vector bundles here as well, then we would have the types here as well. but you already said #193 you want to review that thoroughly, which I can fully understand.

@mateuszbaran
Copy link
Member Author

I think we can merge this now?

Copy link
Member

@kellertuer kellertuer left a comment

Choose a reason for hiding this comment

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

LGTM.

@kellertuer
Copy link
Member

Oh, where does the one line we loose on project coverage? Maybe check real quick, but otherwise merge.

@mateuszbaran
Copy link
Member Author

That lost line looks like a false positive
image

@mateuszbaran mateuszbaran merged commit 2ac6a52 into master Jul 28, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants