Skip to content

Issue2788 #2794

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Aug 2, 2025
Merged

Issue2788 #2794

merged 14 commits into from
Aug 2, 2025

Conversation

JacquesCarette
Copy link
Contributor

@JacquesCarette JacquesCarette commented Jul 30, 2025 •
edited
Loading

This puts back everything that was mistakenly reverted -- closes #2788

@MatthewDaggitt MatthewDaggitt changed the base branch from v2.3-release to master July 30, 2025 23:58
@MatthewDaggitt MatthewDaggitt changed the base branch from master to v2.3-release July 30, 2025 23:58
@MatthewDaggitt
Copy link
Contributor

Thanks @JacquesCarette!

Was hoping to make the release of v2.3 now, but I guess we need to wait for @jamesmckinna's approval for this.

@MatthewDaggitt MatthewDaggitt added this to the v2.3 milestone Jul 31, 2025
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Jul 31, 2025 •
edited
Loading

Review incoming, but as a placeholder
Re: the additions (x signifies approval):

  • Algebra.Construct.Flip.Op
  • Data.Fin.Properties
  • Data.Sum.Relation.Binary.LeftOrder looks good, but possible downstream refactoring wrt variables
  • Data.Sum.Relation.Binary.Pointwise has a spurious anonymous module declaration L111-L114, plus some weirdness in the comment/behaviour wrt variables in ⊎-wellFounded

Re: CHANGELOG
This is trickier, because of the 'anachronism's... will re-review shortly. My suggestions end up still leaving it in a not-completely consistent state!?

jamesmckinna
jamesmckinna requested changes Jul 31, 2025
View reviewed changes
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.

See placeholder comment, plus module-level suggestion.
CHANGELOG comments to follow

jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Jul 31, 2025 •
edited
Loading

I've only made suggestions... in case my commits make things worse! But CHANGELOG definitely needs fixing, and Pointwise could be (rational reconstruction) tweaked a little from the original, given all the time which has elapsed since the original PR...

@jamesmckinna jamesmckinna linked an issue Jul 31, 2025 that may be closed by this pull request
[ bug ] missing commits from v2.0 onwards #2788
Closed
4 tasks
@jamesmckinna jamesmckinna mentioned this pull request Jul 31, 2025
Closed
4 tasks
jamesmckinna
jamesmckinna reviewed Jul 31, 2025
View reviewed changes
@jamesmckinna jamesmckinna mentioned this pull request Jul 31, 2025
Open
@jamesmckinna
Copy link
Contributor

jamesmckinna commented Jul 31, 2025 •
edited
Loading

Question: when/if this is merged into the release-2.3 branch, when/how do we also merge it into master? And what about experimental as well?

@jamesmckinna jamesmckinna mentioned this pull request Jul 31, 2025
Closed
JacquesCarette and others added 2 commits July 31, 2025 14:08
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
baa4a2a 
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
517183d 
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette
Copy link
Contributor Author

If/when this gets merged, we'll have to cherry-pick it, somehow, back onto master and experimental.

JacquesCarette and others added 4 commits July 31, 2025 14:53
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
67d68fc 
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
4381f54 
This was indeed anachronistic from 2.0

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
ba88e57 
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette
resolve issues pointed out by James.
ca8f6e8
@jamesmckinna
Copy link
Contributor

Will re-review in the morning!

jamesmckinna
jamesmckinna requested changes Aug 1, 2025
View reviewed changes
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.

Thanks very much again for this!
We're in the home strait...

  • merge the two Data.Fin.Properties sections of CHANGELOG
  • (possibly) fix names for Data.Sum.Relation.Binary.Pointwise either in place, or else in CHANGELOG

Happy to do a final edit myself to fix these up!

jamesmckinna
jamesmckinna reviewed Aug 1, 2025
View reviewed changes
jamesmckinna
jamesmckinna reviewed Aug 1, 2025
View reviewed changes
@jamesmckinna
Copy link
Contributor

@MatthewDaggitt @JacquesCarette thanks for all your efforts in helping sort out the mess I created.
I'm happy to push the final tweaks, if you are (but I've held off doing so out of... the yips, basically)

@JacquesCarette
Copy link
Contributor Author

I should be able to get to this today.

JacquesCarette and others added 3 commits August 1, 2025 11:10
@JacquesCarette @jamesmckinna
Update CHANGELOG.md
bf9715f 
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
@JacquesCarette
remove now obsolete comment
7f84017
@JacquesCarette
last mistake in CHANGELOG, hopefully fixed now.
812067f
@JacquesCarette
Copy link
Contributor Author

Should be good to go now @jamesmckinna .

jamesmckinna
jamesmckinna approved these changes Aug 1, 2025
View reviewed changes
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.

Fantastic! Thanks so much for persevering... and now I hand back to @MatthewDaggitt

@MatthewDaggitt MatthewDaggitt merged commit 5f8f309 into v2.3-release Aug 2, 2025
@MatthewDaggitt MatthewDaggitt deleted the issue2788 branch August 2, 2025 05:00
MatthewDaggitt added a commit that referenced this pull request Aug 2, 2025
@JacquesCarette @jamesmckinna
@Taneb @Seiryn21 @MatthewDaggitt
Add back accidentally removed lemmas (Issue2788 #2794))
12d99db 
* opposite of a `Ring` [clean version of pr #1900] (#1910)

* punchOut preserves ordering (#1913)

* Wellfounded proof for sum relations (#1920)

* last revert undone by hand

* Remove extra line in CHANGELOG

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

This was indeed anachronistic from 2.0

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* resolve issues pointed out by James.

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* remove now obsolete comment

* last mistake in CHANGELOG, hopefully fixed now.

---------

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
Co-authored-by: Nathan van Doorn <nvd1234@gmail.com>
Co-authored-by: Alice Laroche <60161310+Seiryn21@users.noreply.github.com>
Co-authored-by: matthewdaggitt <matthewdaggitt@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Aug 2, 2025
@MatthewDaggitt @JacquesCarette
@jamesmckinna @Taneb @Seiryn21
Backport changes from 2.3 release to 2.4 (#2799)
f767c72 
* Add back accidentally removed lemmas (Issue2788 #2794))

* opposite of a `Ring` [clean version of pr #1900] (#1910)

* punchOut preserves ordering (#1913)

* Wellfounded proof for sum relations (#1920)

* last revert undone by hand

* Remove extra line in CHANGELOG

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

This was indeed anachronistic from 2.0

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* resolve issues pointed out by James.

* Update CHANGELOG.md

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>

* remove now obsolete comment

* last mistake in CHANGELOG, hopefully fixed now.

---------

Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
Co-authored-by: Nathan van Doorn <nvd1234@gmail.com>
Co-authored-by: Alice Laroche <60161310+Seiryn21@users.noreply.github.com>
Co-authored-by: matthewdaggitt <matthewdaggitt@gmail.com>

* Bring v2.3 release changes accross

* Fix typo

* Fix whitespace

---------

Co-authored-by: Jacques Carette <carette@mcmaster.ca>
Co-authored-by: jamesmckinna <31931406+jamesmckinna@users.noreply.github.com>
Co-authored-by: Nathan van Doorn <nvd1234@gmail.com>
Co-authored-by: Alice Laroche <60161310+Seiryn21@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatthewDaggitt MatthewDaggitt MatthewDaggitt approved these changes

@jamesmckinna jamesmckinna jamesmckinna approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
v2.3
Development

Successfully merging this pull request may close these issues.

[ bug ] missing commits from v2.0 onwards
5 participants
@JacquesCarette @MatthewDaggitt @jamesmckinna @Taneb @Seiryn21