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

+Int≡+ #1028

Merged
merged 5 commits into from
Aug 29, 2023
Merged

+Int≡+ #1028

merged 5 commits into from
Aug 29, 2023

Conversation

timorl
Copy link
Contributor

@timorl timorl commented Jul 25, 2023

This is more of a warmup, since I actually want ·Int≡· (useful to prove QuoQ≡HITQ I think?). But this might also prove useful to prove that, so no effort wasted.

I'm aware of some problems with this, but I'm not sure how to fix them, so I'll be grateful for any comments.

  1. The import looks ridiculous and I'm pretty sure it's not conformant with the contribution guidelines. However, I wasn't able to figure out how to write it in any other way and have all this still work (as imports still cause conflicts I think?).
  2. I'm not sure whether the changes to Iso are ok or if there is a better way of doing a similar thing. Similarly, exporting isoIntℤ might be controversial?
  3. I think at least some of the reasoning should be private, but I'm not sure what the best way of achieving this is – private is kinda obvious, but I think I've also seen some unnamed modules? What should I do with this?
  4. The big comment in line 298 – is this something worth pursuing? Within this PR?
  5. In general I have a feeling there might be a much simpler way of doing all this...

Copy link
Contributor

@jpoiret jpoiret left a comment

Choose a reason for hiding this comment

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

I left some generic style comments. Regarding whether to put some of the reasoning in private, I don't really think it's an issue here

Cubical/Data/Int/MoreInts/QuoInt/Properties.agda Outdated Show resolved Hide resolved
Cubical/Foundations/Isomorphism.agda Outdated Show resolved Hide resolved
Cubical/Data/Int/MoreInts/QuoInt/Properties.agda Outdated Show resolved Hide resolved
ℤ→Int+Int≡+ : ∀ (n m : ℤ) → (ℤ→Int n) +Int (ℤ→Int m) ≡ ℤ→Int (n + m)
ℤ→Int+Int≡+ (pos zero) m =
(ℤ→Int (pos zero)) +Int (ℤ→Int m)
≡⟨ sym (pos0+ (ℤ→Int m)) ⟩
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest indenting the reasoning lines with a different number of spaces to improve readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I changed the formatting into something else entirely, because it felt nicer. Lemme know what you think and if you want a specific different one then tell me what it should be.

Cubical/Data/Int/MoreInts/QuoInt/Properties.agda Outdated Show resolved Hide resolved
@timorl
Copy link
Contributor Author

timorl commented Jul 26, 2023

I also privated a lot of the PR after all, I think they are pretty irrelevant.

Copy link
Contributor

@jpoiret jpoiret left a comment

Choose a reason for hiding this comment

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

Rest looks good imo.

where
posℤ→Int+Int≡+ : ∀ (n : ℕ) (m : ℤ) → (ℤ→Int (pos n)) Int.+ (ℤ→Int m) ≡ ℤ→Int ((pos n) + m)
posℤ→Int+Int≡+ zero m =
(ℤ→Int (pos zero)) Int.+ (ℤ→Int m) ≡⟨
Copy link
Contributor

Choose a reason for hiding this comment

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

That's too funky of a notation wrt. the rest of the library. I feel better with seperate lines for steps, with differing indentation. Also, it's not consistent with +'≡+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Better now? Or is this not what you had in mind?

I also fixed the other series of equalities, I completely forgot it was there before. ^^"

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.

Looks good to me as well - I have only some naming/exporting questions.

predℤ→Int (posneg i) = refl

ℤ→Int+Int≡+ : ∀ (n m : ℤ) → (ℤ→Int n) Int.+ (ℤ→Int m) ≡ ℤ→Int (n + m)
ℤ→Int+Int≡+ n m = (ℤelim (λ n → ∀ (m : ℤ) → (ℤ→Int n) Int.+ (ℤ→Int m) ≡ ℤ→Int (n + m)) posℤ→Int+Int≡+ negsucℤ→Int+Int≡+ n) m
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a name containing something like 'IsHom+' would be better.
And then this fact should be exported as well (maybe even as an homomorphism of abelian groups - that might fit in better somewhere else though).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinda related to the other comment – shouldn't we care about the equality (rather than homomorphism) of abelian groups, as in that the base sets are equal and the operations are equal? I believe we can recover a traditional homomorphism (well, iso-) from this quite easily and it seems conceptually simpler when they are literally equal.

I definitely might be missing some performance issues with this though, I have no idea how these work here.

Oh, to be clear, I am quite happy to change it, but I would also like to understand all this better and adjust my approach for the future. ^^"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to be clear: The equality you proved is exactly the statement that the map ℤ→Int is a homomorphism with respect to +. As you suspect, the performance of a homomorphism proof extracted from an equality of groups is not going to be great, so we should export this one separately.
Regarding conventions, it would be better to turn the equality around. In general, we try to write equations like this in a "evaluating/simplifying/normalizing direction" (meaning f(x+y)=f(x)+f(y) for a homomorphism or x*(y+z)=x*y+x*z for distributivity). This is not a well defined term and there is no reason to do it this way -- it is just about having a convention people can remember.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, the thing I wrote about "iso-" was just silly, sorry. Anyway, exported, renamed, and flipped.

I also rebased this, but in the meantime another thing got merged so that was a tad pointless.

@felixwellen
Copy link
Collaborator

Thanks! Looks all good to me now -> will merge if CI is happy.

@felixwellen felixwellen merged commit 3c45155 into agda:master Aug 29, 2023
1 check passed
@timorl timorl mentioned this pull request Aug 30, 2023
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