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

Consider making RecursiveArrayTools and StaticArrays weak dependencies #656

Closed
kellertuer opened this issue Oct 7, 2023 · 5 comments
Closed
Labels
dependencies Pull requests that update a dependency file discussion

Comments

@kellertuer
Copy link
Member

It might be a bit counter-intuitive to the current structure, where we have these methods with specific manifolds,
but I think for the package it would be neat if these packages that introduce speedups for certain data types would be weak dependencies.

@kellertuer kellertuer added discussion dependencies Pull requests that update a dependency file labels Oct 7, 2023
@kellertuer
Copy link
Member Author

kellertuer commented Dec 14, 2023

I will maybe try this next, though for RecursiveArrayPartition this might be breaking, since previously we exported ArrayPartition.
The rest seems mainly to be reorganising code.

edit: but besides that there is even an interplay between StaticArrays and RecursiveArrayTools (double-ext-necessary) Array is much less work. Will try that tomorrow then and see how much this affects other stuff.

Still this might be considered breaking.

@mateuszbaran
Copy link
Member

This may indeed be a bit breaking so I'd prefer to have it scheduled for the next breaking release instead of rushing it now.

@kellertuer
Copy link
Member Author

I started and noticed it is mainly code-reorganisation, but there might be 2-3 small breaking things

  • ArrayPartition is reexported and that is no longer the case after this change
  • `we might loose a few default implementations that are no longer available when you “just” load Manifolds (since a random function creates an ArrayPartition for example)

So – I will slowly do this, open a PR, but that should really only be merged when we have more reasons for a breaking change – since this decoupling will also not have such a large effect on loading and such.

So I fully agree with your opinion :)

@mateuszbaran
Copy link
Member

We can probably close this because RecursiveArrayTools is now a weak dependency and StaticArrays is not worth extracting at the moment?

@kellertuer
Copy link
Member Author

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants