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

abstractify QuotientAlgebra #857

Merged

Conversation

MatthiasHu
Copy link
Contributor

This PR declares almost all definitions in Cubical.Algebra.CommAlgebra.QuotientAlgebra as abstract in an attempt to reduce long type checking times.

FPAlgebra (the only module that uses QuotientAlgebra so far) had to be slightly adjusted too. A witness inducedHom∘quotientHom had to be added to QuotientAlgebra for an equality that was a definitional equality before.

This replaces #856.

@MatthiasHu MatthiasHu marked this pull request as ready for review July 4, 2022 18:32
@MatthiasHu
Copy link
Contributor Author

MatthiasHu commented Jul 4, 2022

Just a note: I can't see how to "abstractify" FreeCommAlgebra in the same way, because it consists of FreeCommAlgebra.Base and FreeCommAlgebra.Properties, and I think there is no way to let the latter module look through abstract definitions in the first module. :-/

@mortberg
Copy link
Collaborator

mortberg commented Jul 4, 2022

Is it clear that making everything abstract like this is the right way to go? My experience is that constructions should not be opaque, but their properties can often be... Also, the "locking" trick that we use in some files for Z-cohomology could be helpful. It's an alternative to making things abstract

@MatthiasHu
Copy link
Contributor Author

I think to avoid the unfolding of algebra terms, it has to be the construction (_/_) that is abstract. I think it would not help to make only properties abstract?
I will look at the locking trick...

@mortberg
Copy link
Collaborator

mortberg commented Jul 4, 2022

I think to avoid the unfolding of algebra terms, it has to be the construction (_/_) that is abstract. I think it would not help to make only properties abstract? I will look at the locking trick...

Yeah, maybe a locked version of _/_ is what you need. I'm not sure if there's some better place to look than https://agda.github.io/cubical/Cubical.Experiments.ZCohomologyOld.Properties.html#24355, but it's the first I found

@MatthiasHu
Copy link
Contributor Author

MatthiasHu commented Jul 14, 2022

I experimented a bit with the locking trick. In principle it seems very very nice to provide an interface where the user of the module can decide themselves whether they want to see reducing or non-reducing behavior (by plugging in either unlock or some general key : lockUnit). But it seems to be a considerable amount of overhead compared to basically just adding a few abstracts as in the present PR.

For example, the definition of quotientHom currently reads:

  quotientHom : CommAlgebraHom A (_/_)
  fst quotientHom x = [ x ]
  IsAlgebraHom.pres0 (snd quotientHom) = refl
  ...

With the locking method it would be either something like this (taken from here):

  quotientHomWithKey : (key : lockUnit)  CommAlgebraHom A (_/WithKey_ key)
  fst (quotientHomWithKey unlock) x = [ x ]
  IsAlgebraHom.pres0 (snd (quotientHomWithKey unlock)) = refl
  ...

  quotientHom : CommAlgebraHom A _/_
  quotientHom = quotientHomWithKey key

Or something like this (taken from here):

  quotientHomUnlocked : CommAlgebraHom A (_/Unlocked_)
  fst quotientHomUnlocked x = [ x ]
  IsAlgebraHom.pres0 (snd quotientHomUnlocked) = refl
  ...

  quotientHom : CommAlgebraHom A (_/_)
  quotientHom = lock' key
    (λ k  CommAlgebraHom A (lock k _/Unlocked_))
    quotientHomUnlocked

In particular, I don't see how to avoid writing out all the type signatures two (or even three) times, in slightly modified ways. This feels bad to me as they can become relatively long, e.g.:

  injectivePrecomp : (B : CommAlgebra R ℓ) (f g : CommAlgebraHom (A / I) B)
                      f ∘a (quotientHom A I) ≡ g ∘a (quotientHom A I)
                      f ≡ g

@felixwellen
Copy link
Collaborator

felixwellen commented Jul 14, 2022

I'm certainly ok with using 'abstract' for now. Should I review this PR?

@MatthiasHu
Copy link
Contributor Author

MatthiasHu commented Jul 18, 2022

Should I review this PR?

Was this question directed at me? :-)

@felixwellen
Copy link
Collaborator

Yes. With a possibility for anders to intervene.

@MatthiasHu
Copy link
Contributor Author

Oh, ok. Yes, I would be pleased if you could review this PR.

@felixwellen felixwellen self-requested a review July 19, 2022 07:47
@felixwellen
Copy link
Collaborator

I can confirm a very nice type checking speed reduction in FPAlgebra! On my notebook, it used to take 80s and now it is down to 20s - which is mostly spend on deserialization, so I guess there is no immediate need to look for further speed improbements to FPAlgebra.

Copy link
Collaborator

@felixwellen felixwellen left a comment

Choose a reason for hiding this comment

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

Only a couple of minor comments/requests for clarifications. Otherwise this is good to merge.
I think we can work with abstract interfaces like this one and see how it goes.

Cubical/Algebra/CommAlgebra/QuotientAlgebra.agda Outdated Show resolved Hide resolved
Cubical/Algebra/CommAlgebra/QuotientAlgebra.agda Outdated Show resolved Hide resolved
Cubical/Algebra/CommAlgebra/FPAlgebra.agda Show resolved Hide resolved
@MatthiasHu
Copy link
Contributor Author

This is ready to be merged from my perspective.

Copy link
Collaborator

@felixwellen felixwellen left a comment

Choose a reason for hiding this comment

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

Also from mine - looks great!

@felixwellen felixwellen merged commit 4a639fa into agda:master Jul 21, 2022
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

3 participants