Skip to content

fixes #1248 (to_set / xsection)#1249

Merged
affeldt-aist merged 2 commits intomath-comp:masterfrom
affeldt-aist:fixes_1248
Aug 1, 2024
Merged

fixes #1248 (to_set / xsection)#1249
affeldt-aist merged 2 commits intomath-comp:masterfrom
affeldt-aist:fixes_1248

Conversation

@affeldt-aist
Copy link
Member

@affeldt-aist affeldt-aist commented Jun 23, 2024

Motivation for this change

TODO: maybe also rename xsection to xsect, that's clear enough.

Checklist
  • added corresponding entries in CHANGELOG_UNRELEASED.md
  • added corresponding documentation in the headers

Reference: How to document

Reminder to reviewers

@affeldt-aist
Copy link
Member Author

@zstone1 what do you think about replacing the local notations to_set?
surely that does not make proofs shorter,
I was hoping to be able to reuse lemmas about xsection but it is not obvious they help

@affeldt-aist affeldt-aist added this to the 1.3.0 milestone Jul 29, 2024
@affeldt-aist affeldt-aist marked this pull request as ready for review July 29, 2024 11:02
@affeldt-aist
Copy link
Member Author

I think that there are more advantages than disadvantages so I undrafted this PR:

  • to_set and xsection are essentially duplicates (one is Prop, the other is bool)
    so getting rid of one of them is good
  • xsection is more "robust" because it is a definition and is better instrumented with lemmas,
    to_set was a repeated local notation that did not parse well

I think that this offsets the more verbose proofs.

@affeldt-aist affeldt-aist requested a review from zstone1 July 31, 2024 14:51
@zstone1
Copy link
Contributor

zstone1 commented Aug 1, 2024

I agree, the way to_set worked was unsatisfactory. This at least consolidates the definition into one place, rather that redefining it. I'm happier with this (and I wouldn't use xsect, this notation isn't widely used enough for me to worry about saving the 3 characters)

@affeldt-aist affeldt-aist merged commit f89df72 into math-comp:master Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants