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: statically sized Vector type #793

Merged
merged 29 commits into from
Aug 16, 2024
Merged

Conversation

Shreyas4991
Copy link
Contributor

@Shreyas4991 Shreyas4991 commented May 11, 2024

This PR adds a static vector type to batteries akin to Mathlib.Data.Vector with a key difference: It is based on Array instead of List and is designed for efficient operations with while offering a static size . The PR attempts to make the API of Batteries.Data.Vector isomorphic to Mathlib.Data.Vector to the maximum possible extent.

EDIT: This PR also extends all the Array API to Vector
For discussion see this Zulip thread

EDIT: After consideration of the magnitude of the task, I limit this PR itself to porting non-monadic functions from Init.Data.Array.Basic and some surrounding API, as well as the extension theorem. I will create further PRs for adapting monadic functions, relevant theorems, and elements of the Array API already present in Batteries and Init

Depends on

Copy link
Collaborator

@nomeata nomeata left a comment

Choose a reason for hiding this comment

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

Thanks! Looking forward to having that. I wonder how soon we'd like to have it (or a private copy) in core. I certainly wrote code there where I thought it would be easier with this…

I noticed that not all operations are backed by Array operations; at least not all that could be. It seems like it should be the case, shouldn't it?

Can we also get nice #[a,b,c] syntax for terms and patterns? Especially in patterns its nice if that is then a complete pattern.

Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
@Shreyas4991
Copy link
Contributor Author

Changing all operations to array operations is my next goal. This causes quite a bit of proof breakage that will have to be fixed. It doesn't make sense to turn operations like head and cons into array operations I suppose?

@nomeata
Copy link
Collaborator

nomeata commented May 12, 2024

It doesn't make sense to turn operations like head and cons into array operations I suppose?

head certainly! Not sure about cons. Maybe it shouldn't be there? Or at least with a warning.

@nomeata
Copy link
Collaborator

nomeata commented May 12, 2024

(also I should mention that I'm just offering unsolicited opinions here, the maintainers have authority here)

@Shreyas4991
Copy link
Contributor Author

I have started porting the large Array API under Init.Data.Array.Basic. This is a very large file, so I expect this to take some time.

@semorrison semorrison added the WIP work in progress label May 13, 2024
@Shreyas4991
Copy link
Contributor Author

Shreyas4991 commented May 13, 2024

This latest commit also adds a number of docstrings, which should be reviewed

@Shreyas4991
Copy link
Contributor Author

Shreyas4991 commented May 13, 2024

I would like to merge this PR after taking care of any new reviews in order to avoid putting too many things together. Subsequent PRs will:

  1. Add a separate file for monadic defs and their dependents.
  2. Port Array lemmas.
  3. Add insertion and sort defs.

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.

First pass on Vector.Basic.

Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Basic.lean Outdated Show resolved Hide resolved
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.

Review Vector.Lemmas.

Batteries/Data/Vector/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Lemmas.lean Outdated Show resolved Hide resolved
Batteries/Data/Vector/Lemmas.lean Outdated Show resolved Hide resolved
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.

Ready for community review.

@Shreyas4991
Copy link
Contributor Author

awaiting-review

@github-actions github-actions bot added awaiting-review This PR is ready for review; the author thinks it is ready to be merged. and removed WIP work in progress labels May 15, 2024
Comment on lines 12 to 18
/--
`Vector α n` is an `Array α` whose size is statically fixed to `n`
-/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link

@YaelDillies YaelDillies left a comment

Choose a reason for hiding this comment

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

Could you explicitly indicate in the docstrings the cost of each operation? See List.map for what I mean.

@Shreyas4991
Copy link
Contributor Author

Could you explicitly indicate in the docstrings the cost of each operation? See List.map for what I mean.

We maintained those complexity annotations where the corresponding Array docstring stated the complexity. For the rest, at some point, we just agreed that predicting whether an operation is an in-place edit is not trivial.

fgdorais added a commit to fgdorais/batteries that referenced this pull request Jul 19, 2024
@digama0 digama0 merged commit 5f405b1 into leanprover-community:main Aug 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review This PR is ready for review; the author thinks it is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants