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(data/matrix/notation): simp lemmas for constant-indexed elements #4491

Closed
wants to merge 14 commits into from

Conversation

jsm28
Copy link
Collaborator

@jsm28 jsm28 commented Oct 7, 2020

If you use the ![] vector notation to define a vector, then simp
can extract elements 0 and 1 from that vector, but not element 2
or subsequent elements. (This shows up in particular in geometry if
defining a triangle with a concrete vector of vertices and then
subsequently doing things that need to extract a particular vertex.)
Add bit0 and bit1 simp lemmas to allow any element indexed by a
numeral to be extracted (even when the numeral is larger than the
length of the list, such numerals in fin n being interpreted mod
n).

This ends up quite long; bit0 and bit1 semantics mean extracting
alternate elements of the vector in a way that can wrap around to the
start of the vector when the numeral is n or larger, so the lemmas
need to deal with constructing such a vector of alternate elements.
As I've implemented it, some definitions also need an extra hypothesis
as an argument to control definitional equalities for the vector
lengths, to avoid problems with non-defeq types when stating
subsequent lemmas. However, even the example added to
test/matrix.lean of extracting element 123456789 of a 5-element
vector is processed quite quickly, so this seems efficient enough.

Note also that there are two @[simp] lemmas whose proofs are just
by simp, but that are in fact needed for simp to complete
extracting some elements and that the simp linter does not (at least
when used with #lint for this single file) complain about. I'm not
sure what's going on there, since I didn't think a lemma that simp
can prove should normally need to be marked as @[simp].


If you use the `![]` vector notation to define a vector, then `simp`
can extract elements `0` and `1` from that vector, but not element `2`
or subsequent elements.  (This shows up in particular in geometry if
defining a triangle with a concrete vector of vertices and then
subsequently doing things that need to extract a particular vertex.)
Add `bit0` and `bit1` `simp` lemmas to allow any element indexed by a
numeral to be extracted (even when the numeral is larger than the
length of the list, such numerals in `fin n` being interpreted mod
`n`).

This ends up quite long; `bit0` and `bit1` semantics mean extracting
alternate elements of the vector in a way that can wrap around to the
start of the vector when the numeral is `n` or larger, so the lemmas
need to deal with constructing such a vector of alternate elements.
As I've implemented it, some definitions also need an extra hypothesis
as an argument to control definitional equalities for the vector
lengths, to avoid problems with non-defeq types when stating
subsequent lemmas.  However, even the example added to
`test/matrix.lean` of extracting element `123456789` of a 5-element
vector is processed quite quickly, so this seems efficient enough.

Note also that there are two `@[simp]` lemmas whose proofs are just
`by simp`, but that are in fact needed for `simp` to complete
extracting some elements and that the `simp` linter does not (at least
when used with `#lint` for this single file) complain about.  I'm not
sure what's going on there, since I didn't think a lemma that `simp`
can prove should normally need to be marked as `@[simp]`.
@jsm28 jsm28 added the awaiting-review The author would like community review of the PR label Oct 7, 2020
vec_cons x u (bit1 i) = vec_alt1 (vec_cons x u) i :=
rfl

/-- `vec_join u v` joins two vectors of lengths `m` and `n` to produce
Copy link
Member

Choose a reason for hiding this comment

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

I'm slightly surprised this doesn't already exist!

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Despite the doc string there, I definitely needed the extra argument here to give control over definitional equality for the vector length (otherwise there's a problem comparing a vector of length m + 1 + n with one of length m + n + 1 in cons_join).

Copy link
Collaborator

Choose a reason for hiding this comment

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

My own experience with finvec-like structures tells me that the append in this PR is much more practical than the one in #4406. (Applying eq.rec on data is like the definition of DTT hell.)

Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Thanks, this looks very useful!

src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
@Vierkantor Vierkantor added awaiting-author A reviewer has asked the author a question or requested changes and removed awaiting-review The author would like community review of the PR labels Oct 7, 2020
jsm28 and others added 6 commits October 7, 2020 10:47
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
@jsm28 jsm28 added awaiting-review The author would like community review of the PR and removed awaiting-author A reviewer has asked the author a question or requested changes labels Oct 7, 2020
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

I missed a few spots in copying my remarks, sorry! Apart from that, looks good to me.

src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
src/data/matrix/notation.lean Outdated Show resolved Hide resolved
jsm28 and others added 5 commits October 7, 2020 12:05
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Co-authored-by: Vierkantor <Vierkantor@users.noreply.github.com>
Comment on lines +173 to +174
def vec_alt0 (hm : m = n + n) (v : fin m → α) (k : fin n) : α :=
v ⟨(k : ℕ) + k, hm.symm ▸ add_lt_add k.property k.property⟩
Copy link
Member

@eric-wieser eric-wieser Oct 7, 2020

Choose a reason for hiding this comment

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

Does it make sense to define this instead as:

Suggested change
def vec_alt0 (hm : m = n + n) (v : fin m → α) (k : fin n) : α :=
v ⟨(k : ℕ) + k, hm.symm ▸ add_lt_add k.property k.property⟩
def fin.bit0 (hm : m = n + n) (k : fin n) : fin m :=
⟨(k : ℕ) + k, hm.symm ▸ add_lt_add k.property k.property⟩

and then using v ∘ fin.bit0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure what other use there would be for that definition, but if we want it (possibly built on variants adding two different fin values, with or without 1 added as well) then presumably it goes in data.fin.

Copy link
Member

@eric-wieser eric-wieser Oct 7, 2020

Choose a reason for hiding this comment

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

Or perhaps for consistency with the other bit0 functions,

def fin.bit0 {n : ℕ} (k : fin n) : fin (n + n) :=
⟨(k : ℕ) + k, add_lt_add k.property k.property⟩

/-- `hm` provides control of definitional equality -/
def fin.bit0' {m n : ℕ} (hm : m = n + n) : fin m := fin.cast hm.symm ∘ (fin.bit0 k)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm mildly in favour of the fin.bit0/fin.bit0' proposal, but keeping vec_alt0 until we really need fin.bit0 also makes sense to me.

Copy link
Member

@eric-wieser eric-wieser Oct 12, 2020

Choose a reason for hiding this comment

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

If nothing else, v ∘ fin.bit0 seems like a clearer name to me than vec_alt0.

Copy link
Member

Choose a reason for hiding this comment

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

Also at that point, it seems quite desirable to express it as

def fin.bit0 {n : ℕ} (k : fin n) : fin (bit0 n) :=
⟨bit0 (k : ℕ), _⟩

to make the connection between nat.bit0 and fin.bit0 really clear.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see any nat.bit0 definition, only _root_.bit0.

As noted above, using bit0 and bit1 in these definitions ends up meaning the subsequent proofs need to unfold bit0 and bit1 in several places; they're simpler if these definitions don't use bit0 and bit1.

Copy link
Member

Choose a reason for hiding this comment

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

How about as a compromise, we introduce fin.bit0, then as a follow-up I'll try re-expressing the proofs to use bit0 k instead of k + k?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you use fin.bit0 for vec_alt0, then in the context of these lemmas you should have fin.bit1 for vec_alt1; vec_alt0 and vec_alt1 are exactly analogous, extracting alternate elements of an even-length vector.

However, fin.bit0 and fin.bit1 would not be so clearly analogous to each other when seen on their own. fin.bit0 would map fin n to fin (bit0 n), while fin.bit1 would also map fin n to fin (bit0 n); it would not map to fin (bit1 n), which would be an odd inconsistency in the fin context. But if you make fin.bit0 instead map to the smallest fin type that can hold all the required values, i.e. fin (bit0 n - 1), that's no longer what's wanted for vec_alt0. And if you combine such a function with cast_succ to get back to fin (bit0 n) as needed for vec_alt0, you start needing to special-case n = 0 because fin 0 not fin 1 is correct there.

I think of bit0 and bit1 as being about numerals; those definitions outside the context of numerals don't seem very helpful. I can see more possible use for two-argument versions that might be something like fin.add_cast_add and find.add_add_one_cast_add that map fin m and fin n to fin (m + n) (with an appropriate extra argument for control of definitional equality of types). That could e.g. map row and column indices in some context to indices for diagonals, or generally be relevant for some kind of convolution indexed by fin types. But it also wouldn't work with function composition for defining vec_alt0 and vec_alt1 in any such convenient way as fin.bit0 or fin.bit1.

Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes as-is, although the suggestion to move vec_alt0 to fin.bit0 would be nice for consistency. @urkud @eric-wieser, any suggestions?

@jcommelin
Copy link
Member

Thanks 🎉

bors merge

Any slight changes concerning vec_alt0 vs fin.bit0 can be sorted out in a later PR.

@github-actions github-actions bot added the ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) label Oct 15, 2020
@github-actions github-actions bot removed the awaiting-review The author would like community review of the PR label Oct 15, 2020
bors bot pushed a commit that referenced this pull request Oct 15, 2020
…#4491)

If you use the `![]` vector notation to define a vector, then `simp`
can extract elements `0` and `1` from that vector, but not element `2`
or subsequent elements.  (This shows up in particular in geometry if
defining a triangle with a concrete vector of vertices and then
subsequently doing things that need to extract a particular vertex.)
Add `bit0` and `bit1` `simp` lemmas to allow any element indexed by a
numeral to be extracted (even when the numeral is larger than the
length of the list, such numerals in `fin n` being interpreted mod
`n`).

This ends up quite long; `bit0` and `bit1` semantics mean extracting
alternate elements of the vector in a way that can wrap around to the
start of the vector when the numeral is `n` or larger, so the lemmas
need to deal with constructing such a vector of alternate elements.
As I've implemented it, some definitions also need an extra hypothesis
as an argument to control definitional equalities for the vector
lengths, to avoid problems with non-defeq types when stating
subsequent lemmas.  However, even the example added to
`test/matrix.lean` of extracting element `123456789` of a 5-element
vector is processed quite quickly, so this seems efficient enough.

Note also that there are two `@[simp]` lemmas whose proofs are just
`by simp`, but that are in fact needed for `simp` to complete
extracting some elements and that the `simp` linter does not (at least
when used with `#lint` for this single file) complain about.  I'm not
sure what's going on there, since I didn't think a lemma that `simp`
can prove should normally need to be marked as `@[simp]`.
@bors
Copy link

bors bot commented Oct 15, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(data/matrix/notation): simp lemmas for constant-indexed elements [Merged by Bors] - feat(data/matrix/notation): simp lemmas for constant-indexed elements Oct 15, 2020
@bors bors bot closed this Oct 15, 2020
@bors bors bot deleted the vec-cons-bit-simp branch October 15, 2020 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants