Skip to content

Conversation

@Akshobhya1234
Copy link
Contributor

@Akshobhya1234 Akshobhya1234 commented May 14, 2021

  • IsUnitalMagma
  • IsQuasigroup

@Akshobhya1234 Akshobhya1234 changed the title add IsUnitalMagma to Structures.agda Define IsUnitalMagma, IsSemiGroupoid, IsSmallCategory May 15, 2021
@MatthewDaggitt
Copy link
Contributor

Hi @Akshobhya1234, thank you for the PR! Got a few comments:

  1. These structures should go in the correct place in the file. Look for section headings -- Structures with 1 binary operation & 1 element and -- Structures with 1 binary operation, 1 unary operation & 1 element

  2. You will also have to go through all the larger structures in the file (e.g. IsMonoid) and add the corresponding re-exports of IsUnitalSemigroup and IsQuasigroup. Unfortunately this process is quadratic in the number of structures in the file, and is a well-known problem which @JacquesCarette and others are working on.

  3. You should add the bundled versions of them in Algebra.Bundles as well.

@MatthewDaggitt MatthewDaggitt added this to the v1.7 milestone May 16, 2021
@MatthewDaggitt
Copy link
Contributor

Oh and and you'll need to run fix-whitespace as described in HACKING.md

@MatthewDaggitt MatthewDaggitt changed the title Define IsUnitalMagma, IsSemiGroupoid, IsSmallCategory Define IsUnitalMagma and IsQuasigroup May 16, 2021
@Akshobhya1234 Akshobhya1234 changed the title Define IsUnitalMagma and IsQuasigroup Define IsUnitalMagma , IsQuasigroup and IsLoop May 17, 2021
@MatthewDaggitt
Copy link
Contributor

Hi @Akshobhya1234, maybe let's just stick with IsUnitalMagma and IsQuasigroup in this PR and try and get it merged in. We can then tackle IsLoop in a future PR.

I see that you have carried out points 1. and 3. from review, thanks! Do you know what to do for point 2.?

@Akshobhya1234
Copy link
Contributor Author

Hi @Akshobhya1234, maybe let's just stick with IsUnitalMagma and IsQuasigroup in this PR and try and get it merged in. We can then tackle IsLoop in a future PR.

I see that you have carried out points 1. and 3. from review, thanks! Do you know what to do for point 2.?

Hi @MatthewDaggitt I am not sure about point 2. I will remove IsLoop for this PR.

@Akshobhya1234 Akshobhya1234 changed the title Define IsUnitalMagma , IsQuasigroup and IsLoop Define IsUnitalMagma and IsQuasigroup May 18, 2021
@MatthewDaggitt
Copy link
Contributor

@Akshobhya1234 apologies for the delay in replying. Had a paper deadline last week and this slipped off my radar.

What I mean by point 2. is that you will need to construct the new structures you've added in larger structures that contain those structures. For example, look at how IsCommutativeMonoid constructs IsCommutativeSemigroup because IsCommutativeMonoid inherits directly from IsMonoid.

open IsMonoid isMonoid public
isCommutativeSemigroup : IsCommutativeSemigroup ∙
isCommutativeSemigroup = record
{ isSemigroup = isSemigroup
; comm = comm
}
open IsCommutativeSemigroup isCommutativeSemigroup public
using (isCommutativeMagma)

In the same way you should make it so IsMonoid constructs an instance of IsUnitalMagma. You will then need to do suitable renaming in larger structures such as rings and semirings, + then go through bundles and do the same thing.

Does that make sense?

@Akshobhya1234
Copy link
Contributor Author

@MatthewDaggitt Thank you for the specific example. I will look into it. However I see that it is indeed a quadratic problem. I will try to come up with something at the earliest.

@MatthewDaggitt MatthewDaggitt removed this from the v1.7 milestone May 27, 2021
@JacquesCarette
Copy link
Contributor

I don't really have additional comments beyond @MatthewDaggitt -- when are you going to complete the changes that were asked?

@Akshobhya1234
Copy link
Contributor Author

I don't really have additional comments beyond @MatthewDaggitt -- when are you going to complete the changes that were asked?

@JacquesCarette apologies for the delay. I will try and complete it by next week.

@Akshobhya1234
Copy link
Contributor Author

@MatthewDaggitt Kindly let me know if I am missing something. If I open isUnitalMagma in IsMonoid using isMagma, then, in MonoidMonomorphism I get this error

"Cannot resolve overloaded projection isMagma because it is not applied to a visible argument when checking that the expression M.isMagma has type IsMagma ≈₂ "

So I didn't do it as it will fail the make test.

@MatthewDaggitt
Copy link
Contributor

Thanks, that looks good! I've made a few minor tweaks, and if the tests pass then I'll merge it in.

If I open isUnitalMagma in IsMonoid using isMagma, then, in MonoidMonomorphism I get this error

Yes, you don't want to reopen IsUnitalMagma fully because, as you found, it contains definitions that are already in scope from opening IsSemigroup.

@MatthewDaggitt MatthewDaggitt added this to the v1.8 milestone Jun 20, 2021
@MatthewDaggitt
Copy link
Contributor

Tests are passing so merging in! Thanks for the PR @Akshobhya1234, sorry it's taken so long. Hopefully we'll get any future ones you make merged in quicker.

@MatthewDaggitt MatthewDaggitt merged commit aa12b7d into agda:master Jun 21, 2021
@MatthewDaggitt MatthewDaggitt modified the milestones: v1.8, v2.0 Jul 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants