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

Cancel lemmas implicit #262

Merged
merged 1 commit into from
Dec 13, 2018
Merged

Cancel lemmas implicit #262

merged 1 commit into from
Dec 13, 2018

Conversation

ggonthier
Copy link
Contributor

Make trailing implicit parameters of cancel f g and {in D, cancel f g} lemmas maximal, so that these lemmas are easier to pass to canLR, canLR_in, and similar.

@CohenCyril
Copy link
Member

CohenCyril commented Dec 11, 2018

Hi @ggonthier thanks, that's a change I had in mind for a while.
I am a bit surprised not to see any line with canLR (or similar) in this PR, do you reserve it for the next one?
Also, can you squash and rebase on top of master, please?

(Indeed, it is preferred to rebase on top of master rather than to merge master into a branch, for the sake of cleaning the history and not confusing github)

@CohenCyril CohenCyril self-requested a review December 11, 2018 16:30
@CohenCyril CohenCyril added kind: refactoring Issue or PR about a refactoring. (reorganizing the code, reusing theorems, simplifications...) needs: rebase PR which is not rebased: check the target is appropriate (generally master) and rebase on top of it. labels Dec 11, 2018
@anton-trunov
Copy link
Member

Would it make sense to add an entry to ChangeLog in this PR to fix #258? Or should I open a new one?
I guess the proposed entry

	* Documented argument names for functions and lemmas. Renamed some
	  of the arguments for consistency reasons. Changed implicit status from
	  non-maximal to maximal for a number of arguments.

should be changed anyways.

@ggonthier
Copy link
Contributor Author

@anton-trunov , yes, one entry should suffice to cover both PRs, and it should be a little more precise than the on proposed. I'm working on extending slightly the PR to cover uses of the lemmas as suggested by @CohenCyril, and also to include partial function cancellation lemmas.

Like injectivity lemmas, instances of cancellation lemmas (whose
conclusion is `cancel ? ?`, `{in ?, cancel ? ?}`, `pcancel`, or
`ocancel`) are passed to
generic lemmas such as `canRL` or `canLR_in`. Thus such lemmas should
not have trailing on-demand implicits _just before_ the `cancel`
conclusion, as these would be inconvenient to insert (requiring
essentially an explicit eta-expansion).
We therefore use `Arguments` or `Prenex Implicits` directives to make
all such arguments maximally inserted implicits. We don’t, however make
other arguments implicit, so as not to spoil direct instantiation of
the lemmas (in, e.g., `rewrite -[y](invmK injf)`).
We have also tried to do this with lemmas whose statement matches a
`cancel`, i.e., ending in `forall x, g (E[x]) = x` (where pattern
unification will pick up `f = fun x => E[x]`).
We also adjusted implicits of a few stray injectivity
lemmas, and defined constants.
We provide a shorthand for reindexing a bigop with a permutation.
Finally we used the new implicit signatures to simplify proofs that
use injectivity or cancellation lemmas.
@ggonthier
Copy link
Contributor Author

Added usage, partial function cancel (pcancel / ocancel), and some residual injective / cancel lemmas.
Rebased, documented (in Changes, also covering #255), and squashed.

Copy link
Member

@CohenCyril CohenCyril left a comment

Choose a reason for hiding this comment

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

Great!
Fixes #258
Fixes #255

@CohenCyril CohenCyril removed the needs: rebase PR which is not rebased: check the target is appropriate (generally master) and rebase on top of it. label Dec 13, 2018
@CohenCyril CohenCyril merged commit f962d2a into master Dec 13, 2018
@CohenCyril CohenCyril deleted the cancel-lemmas-implicit branch December 13, 2018 14:06
@gares
Copy link
Member

gares commented Dec 13, 2018

odd order is now part of Coq's CI and runs on each PR, so breaking it is not nice.

What I do for elpi is to have Coq CI track a branch other than master that I synchronize with master when I'm sure (it is called coq-master).

We could also improve CI here, so that we test odd-order too before merging.

@ggonthier
Copy link
Contributor Author

I've corrected the issue in odd-order, and also submitted #264 to correct related issues in mxrepresentation.v.

ggonthier added a commit that referenced this pull request Dec 14, 2018
Correct and improve implicits and documentation of MatrixGenField; also corrects changes in #262 to allow for a compatible fix in `odd-order`.
@maximedenes maximedenes added this to the 1.8.0 milestone Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: refactoring Issue or PR about a refactoring. (reorganizing the code, reusing theorems, simplifications...)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants