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

Slice exported from Base #34

Open
bramtayl opened this issue Aug 26, 2022 · 9 comments
Open

Slice exported from Base #34

bramtayl opened this issue Aug 26, 2022 · 9 comments

Comments

@bramtayl
Copy link
Owner

As of 1.9, so figure out what what to do about that...

@ParadaCarleton
Copy link

If I can make a suggestion, it would be nice if:

  1. Any features in this package missing from base were added there--which I believe is just Align--and
  2. This package provides all the Slices code if the Julia version in use is pre-1.9. That way, code relying on the new Slices object can be kept compatible with pre-1.9 just by adding this as a dependency.

@bramtayl
Copy link
Owner Author

bramtayl commented Nov 1, 2022

Thanks for the suggestion! I'm not sure about 1, I think that would be up to base. I haven't checked whether Slices in base does more or less things than the Slices that's here. I'm tempted to just rename the Slices here, but I also don't want to break anyone's code.

@mcabbott
Copy link

mcabbott commented Nov 1, 2022

No great suggestions, but if JuliaLang/Compat.jl#663 is ever merged then Compat will provide the new Slices type.

@ParadaCarleton
Copy link

ParadaCarleton commented Nov 1, 2022

Thanks for the suggestion! I'm not sure about 1, I think that would be up to base. I haven't checked whether Slices in base does more or less things than the Slices that's here. I'm tempted to just rename the Slices here, but I also don't want to break anyone's code.

I think it's nearly identical, but @simonbyrne would know better than me.

@ParadaCarleton
Copy link

ParadaCarleton commented Nov 4, 2022

@bramtayl I've checked, and the only differences are:

  1. The Slices in Base don't implement Align.
  2. The Slices in Base are constructed using eachslice, and don't have a constructor method Slices(array, dims...).
    I'm guessing both the additional constructor and Align would be welcomed as pull requests, but Align can stay here if not.

Making old JuliennedArrays code compatible with 1.9 would just require adding a Slices constructor to base, and then having this package define Slices only for versions before 1.9.

@bramtayl
Copy link
Owner Author

bramtayl commented Nov 4, 2022

Ok. Sure, PR's welcome. If you want to submit a PR to base for align, feel free, but I haven't had to much luck with similar PR's.

@mcabbott
Copy link

mcabbott commented Nov 4, 2022

The eager Align is stack, and my understanding is that people were not so keen on adding a lazy version. While a view from eachcol is almost always as good as a dense array, this is not true of the fragmented view made by things like Align.

@ParadaCarleton
Copy link

The eager Align is stack, and my understanding is that people were not so keen on adding a lazy version. While a view from eachcol is almost always as good as a dense array, this is not true of the fragmented view made by things like Align.

That makes some sense, but what's the problem with having both? Sometimes copying is less efficient than returning a lazy view. Julia provides lazy versions of some other functions, like map vs Iterators.map, for exactly this reason.

@ParadaCarleton
Copy link

Looks like there's not much interest in a PR to base; in that case, I think the package should be able to just implement a Slices constructor following the old syntax that forwards the method to Base, to avoid breaking any packages.

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

No branches or pull requests

3 participants