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

feat: add APIs around Array splitting #125

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hargoniX
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@fgdorais fgdorais left a comment

Choose a reason for hiding this comment

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

Code duplication.

@digama0 digama0 added the awaiting-author Waiting for PR author to address issues label Aug 10, 2023
@hargoniX hargoniX requested a review from fgdorais August 13, 2023 20:45
@hargoniX
Copy link
Collaborator Author

Are there still issues with this branch @digama0?

@kim-em
Copy link
Collaborator

kim-em commented Sep 21, 2023

I am tempted to suggest that these functions should be returning Subarrays, so they can be 0(1).

However Subarray is rather underdeveloped, and has some issues, e.g. leanprover/lean4#2360, so I'm not sure.

@fgdorais
Copy link
Collaborator

Maybe Std should have its own Subarray with a better interface (leanprover/lean4#2164) and without a coercion to Array (leanprover/lean4#2360).

@hargoniX
Copy link
Collaborator Author

So what is the call here? Merge like this (possibly until a Subarray API exists)? Or shall I try to develop a Subarray API for std and then re-implement this in terms of the std Subarray API.

@kim-em kim-em added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed awaiting-author Waiting for PR author to address issues labels Sep 26, 2023
@fgdorais
Copy link
Collaborator

@hargoniX The smart call is: what do you actually need? It's definitely not a great for you to wait on a major array refactor. Do you need this now or can you wait another four or more months until something much better comes along?

@kim-em
Copy link
Collaborator

kim-em commented Sep 27, 2023

My preference would be, assuming that this PR is actually needed somewhere and the performance of this PR is sufficient for that purpose, that we add a comment suggesting that in future when there is a usable Subarray API that these functions be refactored to use it. And otherwise proceed as is.

@digama0
Copy link
Member

digama0 commented Sep 27, 2023

We want the Subarray API to be similarly expansive to the Array API. But that doesn't affect this PR much, unless the author wants to write Subarray versions of these definitions. We can always do it later.

@hargoniX
Copy link
Collaborator Author

@hargoniX The smart call is: what do you actually need? It's definitely not a great for you to wait on a major array refactor. Do you need this now or can you wait another four or more months until something much better comes along?

I'm just adding APIs that I think could be useful I don't use them (yet).

@joehendrix
Copy link
Contributor

I'd be inclined to not include these functions in stdlib. The performance of the split variants is O(n) and so probably not a great choice for code where performance matters. For theorem proving one would also want simplification theorems and related rules.

I'm separately partial to head instead of front since that's common in related libraries like the Haskell vector.

Expanding the subarray API (or theorems) would be great though.

@joehendrix joehendrix removed the awaiting-review This PR is ready for review; the author thinks it is ready to be merged. label Oct 26, 2023
@joehendrix joehendrix added the awaiting-author Waiting for PR author to address issues label Oct 26, 2023
@leanprover-community-mathlib4-bot leanprover-community-mathlib4-bot added the merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author. label Nov 15, 2023
@fgdorais
Copy link
Collaborator

Heartbeat check: is this PR still active? If it is then just give a thumbs up and I will delete this comment.

@hargoniX
Copy link
Collaborator Author

I'm not going to invest more time in it.

@fgdorais fgdorais added help wanted Extra attention is needed and removed awaiting-author Waiting for PR author to address issues labels Jul 23, 2024
@fgdorais
Copy link
Collaborator

Understood. I'm removing awaiting-author and adding help-wanted to signal that this PR is "for adoption".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed merge-conflict This PR has merge conflicts with the `main` branch which must be resolved by the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants