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: Fintype deriving handler #3198

Closed
wants to merge 19 commits into from

Conversation

kmill
Copy link
Contributor

@kmill kmill commented Mar 31, 2023

Adds a handler to derive Fintype instances for non-recursive types without indices. Has an optimized implementation for enum types as well where it produces an underlying list (enumList) of its elements. For non-enum types, the Fintype instance comes from an equivalence to a "proxy type" made from Sum, Sigma, PLift, Empty, and Unit.

Also adds a derive_fintype% elaborator to derive Fintype instances in the middle of terms. This is useful in case, for example, a Fintype instance needs an additional DecidableEq instance. This elaborator is defined in terms of a proxy_equiv% elaborator that constructs the proxy type and the the equivalence from it. The machinery for proxy_equiv% is made to be somewhat configurable.


Open in Gitpod

@kmill kmill added awaiting-review t-meta Tactics, attributes or user commands labels Mar 31, 2023
return true

/-- Derive a `Fintype` instance for enum types. These come with a `toCtorIdx` function.
The strategy is to (1) create a list `enumList` of all the constructors, (2) prove that this
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would make sense to implement a FinEnum instance derive handler here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd suggest making a separate deriving handler for this, since the List isn't so useful for the equivalence.

Note that there's already an inverse function for toCtrIdx defined by the DecidableEq deriving handler for enum types. https://github.com/leanprover/lean4/blob/master/src/Lean/Elab/Deriving/DecEq.lean#L110

Copy link
Member

Choose a reason for hiding this comment

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

Is the list useful for anything? Why do we want a special path for enum types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's significantly less expensive in both space and time, if we're going to do any computations with it. 63 lines seems like a small price to pay for that.

Copy link
Member

Choose a reason for hiding this comment

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

When is the cost paid for computing the elements of a fintype? Every time you use finset.sum, or just once when the instance is added to the environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on what kind of computing you're doing. I think if it's in the compiled code, an instance with no arguments (like for types with no parameters) will be evaluated at program initialization. But otherwise, for VM evaluation or symbolic evaluation (like for Decidable-like inference or fin_cases) I think it's more like the elements get computed from scratch at every use.

so mapping to and from `Cᵢ` requires looking through `i` levels of `Sum` constructors.
Ignoring time spent looking through these constructors, the construction of `Finset.univ`
contributes linear time with respect to the cardinality of the type since the instances
involved compute the underlying `List` for the `Finset` as `l₁ ++ (l₂ ++ (⋯ ++ lₙ))` with
Copy link
Member

Choose a reason for hiding this comment

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

What makes l₁ ++ (l₂ ++ (⋯ ++ lₙ)) quadratic complexity? Isn't it $O(mn)$, where $m$ is the number of constructors and $n$ is the size of each? So for the enum case, $n=1$, and the complexity should be the same.

Copy link
Member

@eric-wieser eric-wieser Mar 31, 2023

Choose a reason for hiding this comment

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

List.append agrees that xs ++ ys is $O(\mathtt{xs.length})$

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the point is that l₁ ++ (l₂ ++ (⋯ ++ lₙ)) takes linear time in the total length of the resulting list. ((l₁ ++ l₂) ++ ⋯ ) ++ lₙ would be quadratic time, which is why it's associated the way it is.

But, there's quadratic time (with respect to n) that's still there from needing to deal with all the Sum constructors. Fintype.ofEquiv maps over the resulting list.

| c (x : Fin n) : Fin x → foo n α
| d : Bool → α → foo n α
```
the proxy type it generates is `Unit ⊕ Bool ⊕ (x : Fin n) × Fin x ⊕ (_ : Bool) × α` and
Copy link
Member

Choose a reason for hiding this comment

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

Can we have proxy_type% to generate this term?

Copy link
Contributor 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 if proxy_type% is very useful, since the type is meaningless without the equivalence. Everything's factored out where it'd be easy to add if anyone wants it, but I'd rather not add it without a clear need for it.

@semorrison semorrison added the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 13, 2023
@semorrison semorrison removed the merge-conflict The PR has a merge conflict with master, and needs manual merging. label Apr 15, 2023
@semorrison
Copy link
Contributor

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Apr 18, 2023
bors bot pushed a commit that referenced this pull request Apr 18, 2023
Adds a handler to derive `Fintype` instances for non-recursive types without indices. Has an optimized implementation for enum types as well where it produces an underlying list (`enumList`) of its elements. For non-enum types, the `Fintype` instance comes from an equivalence to a "proxy type" made from `Sum`, `Sigma`, `PLift`, `Empty`, and `Unit`.

Also adds a `derive_fintype%` elaborator to derive `Fintype` instances in the middle of terms. This is useful in case, for example, a `Fintype` instance needs an additional `DecidableEq` instance. This elaborator is defined in terms of a `proxy_equiv%` elaborator that constructs the proxy type and the the equivalence from it. The machinery for `proxy_equiv%` is made to be somewhat configurable.
@bors
Copy link

bors bot commented Apr 18, 2023

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat: Fintype deriving handler [Merged by Bors] - feat: Fintype deriving handler Apr 18, 2023
@bors bors bot closed this Apr 18, 2023
@bors bors bot deleted the kmill_deriving_fintype branch April 18, 2023 05:32
semorrison pushed a commit that referenced this pull request May 10, 2023
Adds a handler to derive `Fintype` instances for non-recursive types without indices. Has an optimized implementation for enum types as well where it produces an underlying list (`enumList`) of its elements. For non-enum types, the `Fintype` instance comes from an equivalence to a "proxy type" made from `Sum`, `Sigma`, `PLift`, `Empty`, and `Unit`.

Also adds a `derive_fintype%` elaborator to derive `Fintype` instances in the middle of terms. This is useful in case, for example, a `Fintype` instance needs an additional `DecidableEq` instance. This elaborator is defined in terms of a `proxy_equiv%` elaborator that constructs the proxy type and the the equivalence from it. The machinery for `proxy_equiv%` is made to be somewhat configurable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has been sent to bors. t-meta Tactics, attributes or user commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants