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

Make Rels less strict, add a StrictRels for the previous version #365

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Taneb
Copy link
Member

@Taneb Taneb commented Dec 20, 2022

No description provided.

Because it's very strict and I'm defining a non-strict version more in keeping with the conventions of the library
@JacquesCarette
Copy link
Collaborator

Looking nice. What are you still intending to do before making it 'Ready for Review'? And would you like comments now anyways?

@Taneb
Copy link
Member Author

Taneb commented Dec 20, 2022

I'd like comments now definitely, please! I still need to copy the top-level comment from (what is now) Categories.Category.Monoidal.Instance.StrictRels to Categories.Category.Monoidal.Rels.


open import Categories.Category.Dagger
open import Categories.Category.Instance.Rels

open import Data.Product
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put explicit using in as many places as possible, strongly so on stdlib imports.

; sym = swap
; trans = λ { (p₁ , p₂) (q₁ , q₂) → (q₁ ∘′ p₁) , p₂ ∘′ q₂}
}
; ∘-resp-≈ = λ f⇔h g⇔i →
Copy link
Collaborator

Choose a reason for hiding this comment

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

as you're already using pattern-matching lambdas, might as well continue to do so instead of using proj here?

; snd = λ { {inj₂ _} p q → Setoid.trans B q p }
}
}
; ⟨_,_⟩ = λ L R → record
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably should use pattern-matching lambda here too

module _ {o ℓ} where

Rels-Cartesian : Cartesian (Rels o )
Rels-Cartesian : Cartesian (Rels o (o ⊔ ℓ))
Rels-Cartesian = record
Copy link
Collaborator

Choose a reason for hiding this comment

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

So if anyone is going to use this record somewhere else, it is going to be a problem of sorts. Agda doesn't like huge explicit records. It much prefers records full of things that have local names, i.e. given using a where clause. That makes the unifier much happier.

Copy link
Member Author

Choose a reason for hiding this comment

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

How huge is huge here? How much should I split the records up?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not the size of the record, it's the size of the contents. 1-liners are fine, but embedded proofs with > 3 steps should definitely be named instead of inlined.

}

StrictRels-Monoidal : Monoidal (StrictRels o ℓ)
StrictRels-Monoidal = monoidalHelper _ record
Copy link
Collaborator

Choose a reason for hiding this comment

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

these monster records are probably best broken up in pieces too.

If you don't like where, I've also done private blocks above the definition where I put everything.

StrictRels-Monoidal : Monoidal (StrictRels o ℓ)
StrictRels-Monoidal = monoidalHelper _ record
{ ⊗ = record
{ F₀ = λ { (A , B) → A ×.× B }
Copy link
Collaborator

Choose a reason for hiding this comment

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

uncurry ×.× ? The reason to see it that way is that there might be room to re-use properties of ×.× directly, instead of expanding out everything. Yes, all the proofs are trivial, but they are also not 'conceptual'. [This one's more of a suggestion than a must fix]

@JacquesCarette
Copy link
Collaborator

If you give me permission to push to your fork, I can help push this to completion.

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