-
Notifications
You must be signed in to change notification settings - Fork 242
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
Consequences of module monomorphisms #2276
Conversation
Can we have a global comment about the purpose of the PR? IIUC, a |
@jamesmckinna I'm not sure what you're asking for there. I don't think |
I think I'd been trying to understand what you were doing in (something like) the following terms (a lifting property from
But it seems as though what you have is doing something else here? |
As for doing things generically, yes, your point is well-made. But I think the 'lifting' idea above is something to look out for generally when thinking about 'concrete categories' such as |
I think @jamesmckinna is simply asking: can you explain in plain English what you are trying to achieve here? It's not clear just from the code itself. |
What I'm trying to achieve here: I want a quick and easy way to prove that I have a module given that I have a raw module and a monomorphism to some other module. |
Re: the discussion above about |
Looking at the parametrisation of the various constructions, I can't help but see this as further argument in favour of #2252 , but in any case without having to address that issue head-on, you might still consider making all of these definitions inside anonymous modules which would allow you to avoid the [DRY] anti-pattern of But taking #2252 more seriously, your operations coul all be parametrised on If I had more time today, I'd re-draft a specimen instance for comparison... but that will have to wait for now. UPDATED: my proposal founders on the coupling of |
@jamesmckinna I've made things a bit simpler with modules. Thanks for the suggestion! I don't know how much #2252 will help, though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a PROVISIONAL approve review.
Everything looks good, but there seems to be an excess of open
ing of structures which end up not being used... so will nit-pick in what follows...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are largely nitpicks/questions of style.
That said: needs CHANGELOG
!
module _ (+ᴹ-isMagma : IsMagma N._≈ᴹ_ N._+ᴹ_) where | ||
|
||
open IsMagma +ᴹ-isMagma | ||
using (setoid) | ||
renaming (∙-cong to +ᴹ-cong′) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this a bit hard to read/understand; might it be better to 'localise' the names here to N
, as eg,
module _ (+ᴺ-isMagma : IsMagma N._≈ᴹ_ N._+ᴹ_) where
open IsMagma +ᴺ-isMagma
using (setoid)
renaming (∙-cong to +ᴺ-cong)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And perhaps even on L20 above write:
open module N = ... renaming (_≈ᴹ_ to _≈ᴺ_; _+ᴹ_ to _+ᴺ_)
Ordinarily, I'd be again such renaming
, but I find the notation N._+ᴹ_
horrible to read, even if unambiguous...
private | ||
R-ring : Ring _ _ | ||
R-ring = record { isRing = R-isRing} | ||
open IsRing R-isRing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly?
module R = Ring (record { isRing = R-isRing })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, whitespace missing in record { isRing = R-isRing }
!
------------------------------------------------------------------------ | ||
-- Properties | ||
|
||
module _ (+ᴹ-isMagma : IsMagma N._≈ᴹ_ N._+ᴹ_) where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, mutatis mutandis, for LeftSemimoduleMonomorphism
...? Etc.
module _ (+ᴹ-isMagma : IsMagma N._≈ᴹ_ N._+ᴹ_) where | ||
open IsMagma +ᴹ-isMagma | ||
using (setoid) | ||
renaming (∙-cong to +ᴹ-cong) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This time, +ᴹ-cong
is nowhere used... so using (setoid)
would suffice?
Sorry that this has taken so long to review! UPDATED thanks for the thoughtful response to all my nitpicks. I'm happy to proceed with things as you have them, modulo the last few outstanding comments about consistency of locally-introduced |
module M = RawLeftSemimodule M₁ | ||
module N = RawLeftSemimodule M₂ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere you use M
and N
where here you have M₁
and M₂
?
Does it make sense to (try to) standardise on one or the other throughout this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ready to go!
Progess:
Creating this PR now to get feedback as soon as possible. I'm intending to add to it as I have the energy.