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

[ refactor ] use variables in Algebra.Consequences.Base #2592

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

jamesmckinna
Copy link
Contributor

@jamesmckinna jamesmckinna commented Feb 19, 2025

This lifts out the refactoring aspects of #2572 and should be merged first of the two.

It also rectifies two bugs

  • use of deprecated names in Data.Rational.Unnormalised.Properties
  • changing to variables means that the names of implicit parameters now matter; but Base still used the incorrect symbol for the binary operation, which has now been corrected.

NB One (and only one!) knock-on change needed when supplying an implicit Op₂ parameter!
UPDATED: in the Propositional case, instantiating proofs from Setoid with instances of cong should be accompanied by a change in parametrisation from implicit to explicit, because (PropositionalEquality.Core.)cong can't (necessarily) infer its argument, whereas the typechecker can infer _∙_ when deploying general Setoid proofs taking an explicit cong : Congruent₂ _∙_ argument ...

@jamesmckinna jamesmckinna changed the title Issue2502 v2.3 refactor [ refactor ] use variables in Algebra.Consequences.Base Feb 19, 2025
@jamesmckinna jamesmckinna added this to the v2.3 milestone Feb 23, 2025
@jamesmckinna
Copy link
Contributor Author

Hmmm... recent merge conflicts were a bit strange, hopefully fixed now.
Also fixed: bug left behind after splitting the original PR into two

@@ -1848,7 +1848,7 @@ m∸n≤∣m-n∣ m n with ≤-total m n
n ∸ m ∎

*-distribˡ-∣-∣ : _*_ DistributesOverˡ ∣_-_∣
*-distribˡ-∣-∣ a = wlog ≤-total (comm⇒sym[distribˡ] {_◦_ = _*_} ∣-∣-comm a)
*-distribˡ-∣-∣ a = wlog ≤-total (comm⇒sym[distribˡ] ∣-∣-comm {_◦_ = _*_} a)
Copy link
Contributor

@MatthewDaggitt MatthewDaggitt Feb 27, 2025

Choose a reason for hiding this comment

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

This makes this look like not a cosmetic change but a breaking change? And therefore needs to be a v3.0 if we want to do it?

Copy link
Contributor Author

@jamesmckinna jamesmckinna Feb 27, 2025

Choose a reason for hiding this comment

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

Sigh.
The proof above was itself only introduced last week in #2577 ... and I guess that it is inevitable that the move to using variable always has the potential to reorder the order of implicit arguments in proofs. In the past we haven't (apparently? it typically has arisen where there has been a ground argument lifted out as a variable, eg an element of the Carrier of a Setoid, n : ℕ etc. and in general those have been inferrable, so we haven't observed the 'problem') regarded such as breaking behaviour...

But here, it's possibly because the 'real' underlying issue may be that the ambient parameterisation on the binary operation ought to be explicit (cf. Function.Consequences.Propositional). Will investigate whether there's a better fix to the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're probably right it should be v3.0, and then we can fix the parametrisation problems exposed above as well.

Copy link
Contributor Author

@jamesmckinna jamesmckinna Feb 27, 2025

Choose a reason for hiding this comment

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

... and I'd press on the argument in favour of #2572 as well as part of such v3.0 changes...

@jamesmckinna jamesmckinna modified the milestones: v2.3, v3.0 Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants