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

swap the argument order in equational reasoning #999

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

maxsnew
Copy link
Collaborator

@maxsnew maxsnew commented Mar 25, 2023

Addressing #998 .

Currently not building because the current solvers have some hacks in them related to equational reasoning that I haven't fixed because they might not be necessary after this change. It would be nice to have an example of how this helps with a solver. I have a category solver in a different repo that I'm currently using to verify that this helps.

In the stdlib they also changed other *-reasoning syntax. I'm not familiar enough with the stdlib to know where all of those would be. Additionally I haven't change ≡⟨⟩⟨⟩-syntax or _≡⟨_⟩≡⟨_⟩_ yet but they should be similarly adapted as well.

@felixwellen
Copy link
Collaborator

I think the problematic code can be removed safely, since it wasn't in use anyway. I'm just type checking that and will make a PR on your PR once it is through.

felixwellen added a commit to felixwellen/cubical that referenced this pull request Mar 27, 2023
@felixwellen
Copy link
Collaborator

Here it is, just in case it doesn't show up automatically (not checked yet, but I guess it is good to merge into this PR):

maxsnew#1

@felixwellen
Copy link
Collaborator

Type checks on my machine and I actually could just substitute the worst ringsolver-macro with another one, because the RHS is available now in equational reasoning.

@felixwellen
Copy link
Collaborator

As far as I can tell, that should be ready for merging - or do you want to change anything?

@maxsnew maxsnew marked this pull request as ready for review March 28, 2023 12:05
@maxsnew
Copy link
Collaborator Author

maxsnew commented Mar 28, 2023

Looks good to me. Main thing I would want to add is a test that demonstrate it works with equational reasoning

@felixwellen
Copy link
Collaborator

Have you looked at the CommRingSolver/Examples?
That shows it works in simple cases und you can check it won't work in more complicated cases.
But this is due to a shortcoming of how deBruijn-indices are handled in the CommRingSolver.
I think it would be good to rewrite the latter using more existing code for the reflection interface, which is not neccessarily in the cubical-library. So I would say that is out of scope here...

@maxsnew
Copy link
Collaborator Author

maxsnew commented Mar 29, 2023

Oh I guess I missed those. LGTM then

@felixwellen
Copy link
Collaborator

Okay -> merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants