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

[Merged by Bors] - feat: port Data.Fin.VecNotation #1741

Closed
wants to merge 14 commits into from

Conversation

jcommelin
Copy link
Member

@jcommelin jcommelin commented Jan 21, 2023


  • feat: port Data.Fin.VecNotation
  • Initial file copy from mathport
  • automated fixes

Open in Gitpod

Mathbin -> Mathlib

fix certain import statements

move "by" to end of line

add import to Mathlib.lean
@jcommelin jcommelin added help-wanted The author needs attention to resolve issues mathlib-port This is a port of a theory file from mathlib. labels Jan 21, 2023
@Ruben-VandeVelde Ruben-VandeVelde added awaiting-review The author would like community review of the PR and removed help-wanted The author needs attention to resolve issues labels Jan 22, 2023
@urkud
Copy link
Member

urkud commented Jan 23, 2023

I'm going to fix lint errors. UPD: done. Should we backport the only API change? Do we need the whole section about bit0/bit1 or should we just #noalign all these lemmas?

- Add a docstring.
- Drop some `@[simp]` attributes.
- Reformulate `vecAppend_apply_zero` using `[NeZero]`
@jcommelin
Copy link
Member Author

jcommelin commented Jan 23, 2023

bors d=Ruben-VandeVelde

@github-actions github-actions bot added delegated and removed awaiting-review The author would like community review of the PR labels Jan 23, 2023
@Ruben-VandeVelde
Copy link
Collaborator

Should we backport the only API change?

Done in leanprover-community/mathlib#18259

Do we need the whole section about bit0/bit1 or should we just #noalign all these lemmas?

Keep them for now

@eric-wieser
Copy link
Member

Should we backport the only API change?

Why was an API change made in the port? The change itself looks fine, but I don't think we should do this type of thing without at least explaining why in the PR description.

rfl
#align matrix.vec_append_eq_ite Matrix.vecAppend_eq_ite

-- Porting note: proof was `rfl`, so this is no longer a `dsimp`-lemma
Copy link
Member

Choose a reason for hiding this comment

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

Note that it would be a dsimp lemma if we had

def Nat.ble : @& Nat → @& Nat → Bool
  | zero,   _      => true
  | succ _, zero   => false
  | succ n, succ m => ble n m

instead of

def Nat.ble : @& Nat → @& Nat → Bool
  | zero,   zero   => true
  | zero,   succ _ => true
  | succ _, zero   => false
  | succ n, succ m => ble n m

@jcommelin jcommelin added the awaiting-review The author would like community review of the PR label Jan 23, 2023
@urkud
Copy link
Member

urkud commented Jan 23, 2023

Should we backport the only API change?

Why was an API change made in the port? The change itself looks fine, but I don't think we should do this type of thing without at least explaining why in the PR description.

There was a linter error about an unused have. While fixing it, I made this change for no reason better than "we should make this change some day, why not today?".

@jcommelin
Copy link
Member Author

This file is currently blocking progress.

@jcommelin
Copy link
Member Author

@urkud are you ok with reverting the API change and leaving a TODO?

@Ruben-VandeVelde
Copy link
Collaborator

bors r+

@bors
Copy link

bors bot commented Jan 23, 2023

🔒 Permission denied

Existing reviewers: click here to make Ruben-VandeVelde a reviewer

@urkud
Copy link
Member

urkud commented Jan 23, 2023

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review The author would like community review of the PR labels Jan 23, 2023
bors bot pushed a commit that referenced this pull request Jan 23, 2023
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Ruben Van de Velde <65514131+Ruben-VandeVelde@users.noreply.github.com>
Co-authored-by: Yury G. Kudryashov <urkud@urkud.name>
@bors
Copy link

bors bot commented Jan 23, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: port Data.Fin.VecNotation [Merged by Bors] - feat: port Data.Fin.VecNotation Jan 23, 2023
@bors bors bot closed this Jan 23, 2023
@bors bors bot deleted the port/Data.Fin.VecNotation branch January 23, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mathlib-port This is a port of a theory file from mathlib. ready-to-merge This PR has been sent to bors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants