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

Implement ≤-total in terms of _≤?_ #2440

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

Implement ≤-total in terms of _≤?_ #2440

wants to merge 4 commits into from

Conversation

Taneb
Copy link
Member

@Taneb Taneb commented Jul 19, 2024

Fixes #2436

@gallais
Copy link
Member

gallais commented Jul 19, 2024

You'll need a CHANGELOG entry as it's a breaking change for people
who are relying on ≤-total's reduction behaviour on open terms.

@Taneb
Copy link
Member Author

Taneb commented Jul 19, 2024

That's a good point, I'll add a changelog after #2439 gets merged

@JacquesCarette
Copy link
Contributor

I'll wait to do a proper review once the CHANGELOG is in place.

Copy link
Contributor

@jamesmckinna jamesmckinna left a comment

Choose a reason for hiding this comment

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

Modulo:

  • CHANGELOG
  • breaking

This looks great!

@jamesmckinna
Copy link
Contributor

How does this PR contribute to (positively or negatively...) the issues about 'concrete' orderings raised in issue #1179 ?
It seems as though both are trying to tackle a pervasive problem; this PR, locally, with the bigger issue at a (more) global level.

@Taneb
Copy link
Member Author

Taneb commented Aug 30, 2024

@jamesmckinna they're related, but I don't think either of them solve or get in the way of the other

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Aug 30, 2024

Happy to merge once there's a second 'approve' review from one of the other maintainers... and a CHANGELOG entry ;-)

@jamesmckinna
Copy link
Contributor

jamesmckinna commented Sep 1, 2024

I think when you resolve the merge conflict in CHANGELOG, you'll see that your note on this being a non-backwards compatible change has been added to the v2.1 version, and you should bring the whole of that file up-to-date with the current master/v2.2 version...?

src/Data/Nat/Properties.agda Outdated Show resolved Hide resolved
@MatthewDaggitt MatthewDaggitt removed this from the v2.2 milestone Sep 3, 2024
@MatthewDaggitt
Copy link
Contributor

The question is do we view this as a backwards compatible breaking change? If it was just a change in behaviour then I would say yes and bump it to v3.0. If however, we argue that this is in fact a performance bug then the maybe we can justify adding it to v2.2?

@Taneb
Copy link
Member Author

Taneb commented Sep 5, 2024

@MatthewDaggitt while I would like it in sooner, of course, the position that has the better argument is that we don't make any performance guarantees at all (our readme says more or less this right near the beginning). Based on that, it probably ought wait for 3.0

@jamesmckinna
Copy link
Contributor

@MatthewDaggitt while I would like it in sooner, of course, the position that has the better argument is that we don't make any performance guarantees at all (our readme says more or less this right near the beginning). Based on that, it probably ought wait for 3.0

Well, if you were to make that sacrifice, that would ease the path to a v2.2 release following #2473 ...

@Taneb
Copy link
Member Author

Taneb commented Jan 30, 2025

Any objections to this being merged now?

@Taneb Taneb requested a review from gallais January 30, 2025 12:00
@jamesmckinna
Copy link
Contributor

Any objections to this being merged now?

Against our most recent discussion on the topic, we still do have a (placeholder?) non-breaking v2.3 milestone open, whereas breaking issues/PRs are typically being considered as v3.0? So maybe, ready though it may be, we can hold off merging for a bit? or is this mission critical to another project?

@JacquesCarette
Copy link
Contributor

Do we really wish to regard relying on reduction behaviour of proofs as 'breaking'? I completely agree that, given how Agda (MLTT, really) works, lots of proofs inevitably rely on the reduction behaviour of the terms they speak about. But lifting that up a level seems a bit too far? Do we have many theorems about properties of properties?

I suspect that this change would, in fact, not affect anyone. ['suspect' indicates lower confidence that 'bet' as I'm not a big gambler.]

@jamesmckinna
Copy link
Contributor

@JacquesCarette let me clarify my position:

  • my understanding was that we had agreed that this was breaking, but I am happy if we row back on such a judgment
  • my intention in the above was to draw attention to PRs which are clearly non-breaking/v2.3, in contrast to those which might better/best be judged v3.0...

If everyone is happy, I'd merge this for v2.3, but would advise, for belt-and-braces, that there be a "non-backwards-compatible" entry in CHANGELOG to record what's happening?

@jamesmckinna
Copy link
Contributor

Or else, that we abandon the distinction v2.3/v3.0, and fast-forward to admit all the breaking changes? (We may have agreed that at the last maintainers' meeting, but for whatever reason, I'm dragging my heels...)

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.

Data.Nat.Properties.≤-total should be implemented with _<ᵇ_
5 participants