-
Notifications
You must be signed in to change notification settings - Fork 239
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 Algebra.Solver.*Monoid
(further!)
#2457
base: master
Are you sure you want to change the base?
Conversation
Algebra.Solver.*
refactoringAlgebra.Solver.*Monoid
refactoring
Algebra.Solver.*Monoid
refactoringAlgebra.Solver.*Monoid
(further!)
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.
Nice.
But what I really meant was that at least CommutativeMonoid/Normal
and IdempotentCommutativeMonoid/Normal
are special cases of a single normal form over a ring. So those two pieces of code can be merged.
Also, in Monoid/Normal
, while you define NF
as a (raw) Monoid, you don't use that anywhere. In fact, lower down, you use the list explicitly, instead of taking the information out of the NF
that you've just defined.
@JacquesCarette you wrote:
Oh, I think that I didn't yet see that commonality, only that they share some structure in common, but the UPDATED: In other words, there should be a construction in
Also true (I think)... except that in this refactoring, do I not (implicitly) invoke the monoid operation as being given by the NB. Also, I am only refactoring by re-using existing code that was already there... without (much) attempt to exploit it for 'better' proofs, more like doing a grandiose CSE on the whole thing... so there's (obviously) potentially still room for improvement! |
Re: discussion of potential I guess what I can now glimpse is that the construction above of the form
should better be phrased in terms of a Semiring, in the one case, the Is that what you were intending? |
Yes! |
Going back to DRAFT while I figure out the And a bit more further thought suggests that the construction at hand is most general when presented with one UPDATED: I'm finding this a bit hard to get my head round (esp. why is the |
Might be best to not worry about |
But still: are you suggesting to go via #2407 , or via the more elaborate refactoring here? |
I prefer this one. |
Given @MatthewDaggitt 's endorsement of #2407 I'm going to suggest that we go with that one for now, and then follow-up with this one once the infrastructure is in place to fulfil the |
Following a suggestion by @JacquesCarette on #2407 . Also fixes #2403
NB.
Expr
as backquoted versions of theRawMonoid
signature; deprecates the 'old' syntaxCHANGELOG
, but would be substantially that of above PRThis perhaps represents the "subsequent downstream refactoring" alluded to in that PR, at least as regards further improving the 'shared' interface offered/afforded by the API for
Normal
terms... for the cost of supplyingIsMonoid
andIsMonoidHomomorphism
proofs upfront.Possibly a 'better' version to go for straightaway rather than going via 'transitional' #2407 ? (NB. If so, need to cherry pick the other tweaks to the various related
Solver
modules...)