-
Notifications
You must be signed in to change notification settings - Fork 195
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
Wildcat rewriting tweaks #2208
base: master
Are you sure you want to change the base?
Wildcat rewriting tweaks #2208
Conversation
@Alizter I fixed some of the simpler breakages that result from this change but there's a proof that breaks downstream about classifying spaces and I don't really know how to fix it. It's a unification failure, I guess I can conjecture that since I removed one hint, another one was used to solve typeclass inference in this scenario, and it returns the "wrong" result and so the elaborated hint is incompatible with the goal. Edit: this is fixed now. |
@patrick-nicodemus Can you explain the motivation for the two Hint Mode commands? In this library they seem to make a couple of things more awkward, so it would be good to know what things are made easier. Also, can you explain the choices of -, + and ! in those commands and what they will do? |
@jdchristensen Hi, yeah. I was going to do this but I talked to Ali C. about it privately and forgot to post an explanation publicly. This is a change to how typeclass inference will behave when it's trying to solve a goal of the form in So, I was trying to build a natural transformation the other day and I tried
Now compare:
I call this "inconsistent" because I would expect both to generate subgoal I view the behavior in the
Calling I think that this can be blamed on this non-backtracking behavior of typeclass resolution when it resolves unification variables. I'm not sure if this can be configured. What can be configured is to simply tell typeclass resolution not to try and resolve certain unification variables, and this is what So, I have weakened typeclass inference so that hints like The other line of code referencing |
@patrick-nicodemus , thanks for the detailed explanation! That is strange behaviour that you show in the two examples. I noticed that if you add The loop you describe involving I don't completely understand the two hints you added, with one being on the Class and one being on an Instance. I also don't understand your comment The impact on the library is the main thing I don't understand. E.g. take line 379 of Adjoint.v. Before this change, both I'm also not sure why line 584 of ClassifyingSpace.v needed to change. If a "wrong" hint is being found, it would be good to know what it is and see if we can prevent the problem instead of working around it. That said, that line in the master branch was slow, and your proposed line is much faster. And it can be made a bit cleaner looking: rapply (is1natural_prewhisker (G:=opyon X) B (opyoneda _ _ _)). Neither of these are deal breakers, but it would be nice to know why this change makes some things fail that worked before. |
Yes, I forgot to push it after making the commit, sorry. |
The change has both positive and negative effects, I think. I described the intended positives in the post. The negative is that it weakens typeclass inference by making these hints inapplicable when it would be convenient and appropriate for them to be applicable. I believe that the infinite loops are due to exposing pre-existing cycles in the typeclass hint database - it's not allowed to apply the appropriate hint which would solve the problem, so it goes to the next hint, and eventually discovers a cycle. I debugged one of these and the cycle was apparently due to typeclass hints |
From the documentation for that flag it seems that
to every class defined in the file. I tested this by swapping one for the other in the definition of
|
@patrick-nicodemus Thanks for pointing out the obvious explanation of why this doesn't happen with And thanks for pointing out that loop in typeclass search. We try to avoid any possibilities of loops. The library builds if we use I was looking at the loop on line 379 of Adjoint.v if For that spot in ClassifyingSpace.v, with the original
which is a wild goose chase. But with the hint, it still fails to choose My summary of the situation is that Hint Mode settings cause legitimate searches to fail, and that because of the choices of instances in the WildCat library, this then leads to fruitless searches. So I'm hesitant to add these Hint Mode commands unless I see ways in which they improve the library. @Alizter It seems like the typeclass search we have in WildCat has too any instances that cause unnecessary search when the "obvious" solutions aren't found (e.g. when they are limited by |
Co-authored-by: Dan Christensen <[email protected]>
Something non-obvious that I should state is that multiple hint modes can be given for a term. I believe they are all tried in order. That might help with obvious ones that are not picked up. I haven't been able to check in detail however. Regarding there being too many typeclass instances, I agree. We can and should try pruning them. Previous attempts ran into proofs breaking in unexpected ways which makes this line of exploration less appetising. There is a setting for typeclasses which requires a unique solution. However we can't have multiple typeclass databases which means things like truncation overlap and fail. It's not clear what the best way forward with this is. |
I'm not sure yet whether we should go ahead with this change. The diff is now pretty small, so it's not a big deal to incorporate it, but it's hard to say whether it will help or hinder future work using natural transformations. If we do go ahead, I would suggest two things:
|
Yes, I concur that we should experiment a bit more with this. I still don't have a proper understanding of the situation but I will take some time to do so soon. |
Using Hint Mode to change undesirable behavior of rapply Build_NatTrans