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

Chore: Replace open scoped Classical with (open scoped Classical in) or (classical) #20501

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

Conversation

kvanvels
Copy link
Collaborator

@kvanvels kvanvels commented Jan 6, 2025

reduces some technical debt by replacing "open scoped classical" with (open scoped classical in) for theorem declarations and/or adding a "classical' tactic to a tactic proof. I followed the example in PR #20325.


Open in Gitpod

@sgouezel
Copy link
Contributor

Personally, I would put it at the start of the proof for simplicity (it's not that it enables classical reasoning, which is always enabled in fact, it's that it provides decidability assumptions for some lemmas that need it, so it's mostly a technical implementation detail IMHO). But putting it in the middle of the proof as you did is also fine.

@kvanvels
Copy link
Collaborator Author

Hmmm, ok. So I was mixing up classical reasoning (the axiom of choice and its consequences) and decidability/computability?

@kvanvels
Copy link
Collaborator Author

I think committed at faux pas. I did some more work in removing some "Classical"s this morning (in new files) and I added them to this pull request. I modified some pretty basic (close to the root) files and my computer was taking forever to compile them. I pushed them here (without thinking it through) to have the servers here compile them.

Should I make a new branch to continue fixing these "classical"s?

@sgouezel
Copy link
Contributor

sgouezel commented Jan 14, 2025

It's not really a faux pas, but the PR is reaching a critical size, so I think it is a good point to stop there and wait for a review. If you want to go on working on this, you can do it on another branch (either based on this one, or from master).

@sgouezel
Copy link
Contributor

sgouezel commented Jan 14, 2025

You can also see that there is an "Awaiting-author" label on the PR. This means that the PR will not show up on the reviewers dashboard currently. So, once you are happy with the state of your PR, it is important that you remove this tag, to indicate that your PR is ready for review.

@kvanvels kvanvels removed the awaiting-author A reviewer has asked the author a question or requested changes label Jan 14, 2025
@kvanvels
Copy link
Collaborator Author

There was one final request from your initial review that I just fixed. Am I correct in assuming that that is what removed the "awaiting-author" tag on the PR?

@j-loreaux
Copy link
Collaborator

No, you removed the awaiting-author tag (probably by clicking on it). This is fine if and only if you're ready for the PR to be reviewed again, which it seems is the case because all the conversations are marked as resolved.

Copy link
Collaborator

@j-loreaux j-loreaux left a comment

Choose a reason for hiding this comment

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

some pretty minor comments.

Archive/Wiedijk100Theorems/Partition.lean Show resolved Hide resolved
Mathlib/Computability/TuringMachine.lean Outdated Show resolved Hide resolved
Mathlib/Computability/TuringMachine.lean Outdated Show resolved Hide resolved
Mathlib/Computability/TuringMachine.lean Outdated Show resolved Hide resolved
Mathlib/Probability/Process/HittingTime.lean Outdated Show resolved Hide resolved
@j-loreaux j-loreaux added the awaiting-author A reviewer has asked the author a question or requested changes label Jan 14, 2025
@kvanvels kvanvels removed the awaiting-author A reviewer has asked the author a question or requested changes label Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-contributor This PR was made by a contributor with at most 5 merged PRs. Welcome to the community!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants