-
Notifications
You must be signed in to change notification settings - Fork 246
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
[ refactor ] (Re)define (Is)TightApartness
and (Is)HeytingCommutativeRing
/(Is)HeytingField
#2588
base: master
Are you sure you want to change the base?
Conversation
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.
Otherwise, code-wise it looks good. Can't comment on the mathematics 😄
@@ -17,6 +17,10 @@ Non-backwards compatible changes | |||
significantly faster. However, its reduction behaviour on open terms may have | |||
changed. | |||
|
|||
* The definitions of `Algebra.Structures.IsHeyting*` and |
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 know this is a draft, but in the final version it would be good to explain broadly what the refactorings actually are here?
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.
See latest commits, hopefully enough, but not too much, detail now!
… re-exported by `Algebra.Properties.Ring`
I think this is now ready for review! Many thanks to @MatthewDaggitt for the pre-review, hopefully the latest round of (many!) commits have put things on a more sound footing. As in updated preamble above:
|
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.
Spent a lot of time reviewing this... and no comment to speak of. I did spot several proofs that I'd want to change, but I should add some reasoning combinators first that would show how these embody repeated patterns.
Hopefully the discussion on #2587 clarifies this? |
I'll return to the merge conflicts after v2.3, given that this is a breaking change. |
Fixes #2587
Adds:
Algebra.Apartness.Properties.HeytingField
, supersedingAlgebra.Apartness.Properties.HeytingCommutativeRing
NB:
Algebra.Properties.Ring
(or evenAlgebra.Properties.AbelianGroup
...) DONE properties moved; module NOT deprecatedData.Rational.Unnormalised.Properties
should be refactored to make use ofRelation.Binary.Properties.DecSetoid
for its(Is)*ApartnessRelation
definitions DONEIssue:
bug
fix of the various APIs involved?) or v3.0 (as a largebreaking
change?)Apartness
vs.TightApartness
, etc. but that the fundamental distinction should be between*CommutativeRing
and*Field
(and yes: perhapsHeytingLocalRing
would have been better, but see also DefineLocalRing
#2219 ) in that the latter should have inverses, while the former need not. The existing APIs make the distinction turn on whether the apartness is tight or not, which seems 'wrong'... even to the point of being abug
?RFC from @cspollard and @bsaul ...