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

[WIP] Rework of allocations #78

Closed
wants to merge 1 commit into from
Closed

Conversation

mateuszbaran
Copy link
Member

This is my semi-broken attempt at improving out allocation methods. I'm not sure if this is worth pursuing -- very little is simplified and other places get more complicated. It also shows how most currently existing methods of allocate_result are poorly design in some way but fixing it is a really tedious task.

@kellertuer
Copy link
Member

Would it then maybe be reasonable to think of a new way to do the allocation? The main challenge I think is – in the example of the log – to allocate a tangent vector when no tangent vectors are among the input parameters?

I have not yet given it so much of thoughts, but in general found the current approach ok to work with.

@mateuszbaran
Copy link
Member Author

That's one challenge. There is also the problem of embedded manifolds where we have to differentiate between ambient space representation and embedded manifold representation (where we have AnotherPlaneManifold that violates the principle that AbstractEmbeddedManifolds have the same representation as their embeddings), and for example power manifold is annoying as usual, because we should access arguments of allocation functions in different ways depending on whether it's coefficients-like, point-like or vector-like.

Of course, the current approach doesn't solve all of these problems but they are relatively niche and it's not that hard to patch an issue when it's actually encountered. I wanted to see how complicated this rework would be so I tried it but it doesn't seem to be worth the trouble.

@kellertuer
Copy link
Member

Ok, is there still a doable way to reduce the current ambiguities?

@mateuszbaran
Copy link
Member Author

Sort of yes, it just requires solving them one by one: writing a new method, and then making a test which is sometimes not straightforward.

@kellertuer
Copy link
Member

Is this still something we should follow up on? Or is this obsolete after 3 years?
I am by far not an expect on the crazy allocation parts we have, nor how good they work. I just feel whenever I call it, I get what I want: Memory for a point or vector of right type :)

@mateuszbaran
Copy link
Member Author

I think this PR is not worth continuing because other PRs have made some smaller improvements. The current system is not perfect but its deficiencies show up so rarely that I'm not very motivated towards improving it.

@kellertuer
Copy link
Member

I have no clue where the imperfections lie, but if a discussion help, we can have one about that for sure. I feel it works very well overall.
But I can understand if something works reasonably well and a rework would be a lot of work, then it is maybe not worth spending the time.

@mateuszbaran
Copy link
Member Author

The main deficiency is in the number of allocate_result and allocate_result_type methods that need to be implemented for manifolds with custom point and tangent vector types, and that it's not entirely clear which ones are necessary in what situations. The current approach is "let's keep adding them until things work" and so far a few methods per manifold were enough but I can imagine situations in which it can blow up to dozens. But that's only a potential future problem, not something we have now.

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.

2 participants