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

Quotient category #653

Merged
merged 8 commits into from
Dec 15, 2021
Merged

Quotient category #653

merged 8 commits into from
Dec 15, 2021

Conversation

barrettj12
Copy link
Contributor

No description provided.

Copy link
Collaborator

@mortberg mortberg left a comment

Choose a reason for hiding this comment

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

I had some suggested changes, but then I remembered your issue which answered my questions: #656

I think all of those lemmas should be generalized and I don't think there is a deep reason they are as they currently are. I would suggest you make the generalizations to the set quotient code and then rewrite the quotient category code in this PR.

Cubical/Categories/Constructions/Quotient.agda Outdated Show resolved Hide resolved
Cubical/Categories/Constructions/Quotient.agda Outdated Show resolved Hide resolved
@barrettj12
Copy link
Contributor Author

Okay, so I generalised elimProp2, elimProp3, elimContr2 and rec2 in SetQuotients.Properties, and then could make my proofs significantly shorter in Categories.Constructions.Quotient.

I decided not to mess with setQuotUniversal2Iso/setQuotBinOp, as I would have had to add extra arguments, and I figure that would be a huge pain.

However, now when I try to compile Algebra.CommRing.Localisation.UniversalProperty, I get an error:

Failed to solve the following constraints:
  Resolve instance argument _1835 : _r_1834 ∈ Aˣ
  Candidates
    _ :
      Σ (Cubical.Algebra.CommRing.Properties.Units.R A')
      (λ r' → fst φ s ·A r' ≡ 1a)
    _ :
      Σ (Cubical.Algebra.CommRing.Properties.Units.R A')
      (λ r' → fst φ s ⁻¹ ·A r' ≡ 1a)
    (stuck)
  _1835 .fst = PathToS⁻¹R.φS⊆Aˣ cond s s∈S' .fst : fst A'
    (blocked on _1835)

I suppose this wasn't happening before, so I must have broken something?

@barrettj12 barrettj12 requested a review from mortberg December 2, 2021 22:11
@ecavallo
Copy link
Collaborator

ecavallo commented Dec 2, 2021

Oops, we did the same thing at the same time: #660

@barrettj12
Copy link
Contributor Author

Oops, we did the same thing at the same time: #660

My bad, should have seen that you self-assigned it. Yours looks more extensive anyway, I'll see if I can merge it in to mine. Does everything compile okay?

@ecavallo
Copy link
Collaborator

ecavallo commented Dec 2, 2021

Everything worked for me, though we'll see what the CI thinks... I'll see if I can find out what's happening with those instance arguments.

@barrettj12
Copy link
Contributor Author

However, now when I try to compile Algebra.CommRing.Localisation.UniversalProperty, I get an error:

Well, the CI on this PR just passed, so I'm not sure why I'm getting this error locally. Maybe I'm using a different version of Agda.

@ecavallo
Copy link
Collaborator

ecavallo commented Dec 2, 2021

I tried locally myself and also had no problem, so that might be it.

@mortberg
Copy link
Collaborator

I merged #660 now. Please fix the conflicts and use the new lemmas in this PR. Please @ me when this is done and I'll take a look

@barrettj12
Copy link
Contributor Author

@mortberg should all be good now.

@mortberg
Copy link
Collaborator

The proofs look really nice now! I'll merge on the CI is finished

@mortberg mortberg merged commit fc6e28d into agda:master Dec 15, 2021
@barrettj12 barrettj12 deleted the quo-cat branch December 15, 2021 15:31
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.

3 participants