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

cleanup in FinNat #2138

Merged
merged 14 commits into from
Nov 17, 2024
Merged

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Nov 14, 2024

We had a lot of universes being generated here, I've cut them down a bit by using Universe Minimization ToSet.

Signed-off-by: Ali Caglayan <alizter@gmail.com>

<!-- ps-id: a4ce60d4-3714-472e-9609-8a55793244be -->
Copy link
Collaborator

@jdchristensen jdchristensen left a comment

Choose a reason for hiding this comment

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

Are the Monomorphic modifiers still needed?

(Also, I don't get why there are various path_* lemmas like path_zero_finnat which are just aliases for path_ishprop. Can't path_ishprop just be used?)

@jdchristensen
Copy link
Collaborator

I got rid of the unneed path_*_finnat definitions, and adapted path_succ_finnat to fit better what is needed. I also simplified a couple of proofs. I suspect finnat_ind could be made to compute better.

I also got rid of one unneeded universe variable in finnat_ind, and annotated a few things to enforce that universe variables don't accidentally arise again.

I didn't think about why some things are marked Monomorphic. Can we just get rid of that?

@Alizter
Copy link
Collaborator Author

Alizter commented Nov 16, 2024

@jdchristensen I think that was misused to get things to not be polymorphic, but that's already the case when we have no Univ variables, therefore it's fine to remove.

I also had some patches cleaning up things here similarly to what you did, but I'll need to check in detail later as I may have changed some proofs differently.

@jdchristensen
Copy link
Collaborator

Ok, I removed Monomorphic. We still use it in other places, e.g. things like Funext are marked as monomorphic even though they don't take any universe variables. I'm not sure what advantage adding Monomorphic gives if something already doesn't use any universe variables, but maybe there's some tiny speedup from this?

@jdchristensen
Copy link
Collaborator

PS: Sorry if I interfered with other patches you had queued up!

Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
Copy link
Collaborator Author

Alizter commented Nov 17, 2024

Ok, I removed Monomorphic. We still use it in other places, e.g. things like Funext are marked as monomorphic even though they don't take any universe variables. I'm not sure what advantage adding Monomorphic gives if something already doesn't use any universe variables, but maybe there's some tiny speedup from this?

I am not sure if there is a speed up, but its probably not worth confusing readers.

PS: Sorry if I interfered with other patches you had queued up!

No worries, I've pushed some of the changes I was going to do while we were here.

@Alizter Alizter changed the title reduce universes in FinNat cleanup in FinNat Nov 17, 2024
Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@jdchristensen
Copy link
Collaborator

This looks good to me, modulo the build error and the one comment about succ_finnat.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter
Copy link
Collaborator Author

Alizter commented Nov 17, 2024

I've fixed the universe issue in 8.19 (for some reason idpath wasn't being minimized to idpath@{Set} and I've reverted the removal of path_succ_finnat like you suggested.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter merged commit 0200dde into HoTT:master Nov 17, 2024
6 checks passed
@Alizter Alizter deleted the ps/rr/reduce_universes_in_finnat branch November 17, 2024 23:30
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.

2 participants