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

[Merged by Bors] - fix(data/complex/basic): ensure algebra ℝ ℂ is computable #8166

Closed
wants to merge 4 commits into from

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jul 1, 2021

Without this complex.ring instance, ring ℂ is found via division_ring.to_ring and field.to_division_ring, and complex.field is non-computable.

The non-computable-ness previously contaminated distrib_mul_action R ℂ and even some properties of finset.sum on complex numbers! To avoid this type of mistake again, we remove noncomputable theory and manually flag the parts we actually expect to be computable.


Open in Gitpod

Without this instance, `normed_field.to_normed_space` and `normed_space.to_module` is tried first, but this results in a noncomputable instance.
@github-actions github-actions bot added the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jul 1, 2021
@eric-wieser eric-wieser changed the title fix(data/complex/basic): ensure algebra ℝ ℂ is computable fix(data/complex/basic): ensure algebra ℝ ℂ is computable Jul 1, 2021
@sgouezel
Copy link
Collaborator

sgouezel commented Jul 1, 2021

Sorry, but isn't everything about real and complex numbers non-computable already? What are you tring to gain here? (I realize it can sound aggressive, but it's not meant to be, that's a real question).

@eric-wieser
Copy link
Member Author

eric-wieser commented Jul 1, 2021

No, only has_inv, decidable_eq and decidable (<) are noncomputable on the reals. Unfortunately lean often finds the addition starting from the noncomputable field structure, which makes lots of things unnecessarily non-computable.

@sgouezel
Copy link
Collaborator

sgouezel commented Jul 1, 2021

Yes, but what is gained by Lean realizing that addition is computable on the complexes (apart from the fact that you don't need to mark any other stuff as noncomputable)?

@eric-wieser
Copy link
Member Author

The motivation was primarily to avoid having to mark things noncomputable erroneously: especially since the existing shortcut instances for real already handle this correctly in almost all cases.

Is it possible to construct some decidably-equal subtype of the complex numbers though, such as the submonoid containing the cube roots of 1? If so, then you can compute with things like zmod 3 ≃+ multiplicative cbrts_one, which could end up noncomputable simply due to the bundling of the equiv.

Is there any harm in adding the instances in this PR, even if the use-case seems largely academic?

@sgouezel
Copy link
Collaborator

sgouezel commented Jul 1, 2021

I don't think there's any harm. I am not convinced there is a real gain either, globally it's probably +\epsilon so still slightly positive :-)

@eric-wieser eric-wieser added the awaiting-review The author would like community review of the PR label Jul 2, 2021
Copy link
Collaborator

@Vierkantor Vierkantor left a comment

Choose a reason for hiding this comment

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

Since you already have done the work, I see no disadvantage to merging this.

@github-actions github-actions bot removed the blocked-by-other-PR This PR depends on another PR which is still in the queue. A bot manages this label via PR comment. label Jul 7, 2021
@github-actions
Copy link

github-actions bot commented Jul 7, 2021

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@github-actions github-actions bot added the merge-conflict Please `git merge origin/master` then a bot will remove this label. label Jul 7, 2021
@eric-wieser
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Jul 7, 2021
Without this `complex.ring` instance, `ring ℂ` is found via `division_ring.to_ring` and `field.to_division_ring`, and `complex.field` is non-computable.

The non-computable-ness previously contaminated `distrib_mul_action R ℂ` and even some properties of `finset.sum` on complex numbers! To avoid this type of mistake again, we remove `noncomputable theory` and manually flag the parts we actually expect to be computable.
@github-actions github-actions bot added ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.) and removed awaiting-review The author would like community review of the PR merge-conflict Please `git merge origin/master` then a bot will remove this label. labels Jul 7, 2021
@bors
Copy link

bors bot commented Jul 7, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title fix(data/complex/basic): ensure algebra ℝ ℂ is computable [Merged by Bors] - fix(data/complex/basic): ensure algebra ℝ ℂ is computable Jul 7, 2021
@bors bors bot closed this Jul 7, 2021
@bors bors bot deleted the eric-wieser/complex.algebra-computable branch July 7, 2021 18:37
@Vierkantor
Copy link
Collaborator

(I didn't do bors d+ yet, because I would have liked to check how things looked after the merge. But everything looks fine, and a lot smaller diff in the end than I was expecting, so no further complaining from me.)

@eric-wieser
Copy link
Member Author

Oh, whoops!

bors bot pushed a commit that referenced this pull request May 3, 2022
This was fixed once before in #8166 (5f2358c), but a new noncomputable shortcut appears if your file has more imports.
bors bot pushed a commit that referenced this pull request May 3, 2022
This was fixed once before in #8166 (5f2358c), but a new noncomputable shortcut appears if your file has more imports.
bors bot pushed a commit that referenced this pull request May 3, 2022
This was fixed once before in #8166 (5f2358c), but a new noncomputable shortcut appears if your file has more imports.
bors bot pushed a commit that referenced this pull request May 3, 2022
This was fixed once before in #8166 (5f2358c), but a new noncomputable shortcut appears if your file has more imports.
bors bot pushed a commit that referenced this pull request May 3, 2022
This was fixed once before in #8166 (5f2358c), but a new noncomputable shortcut appears if your file has more imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge All that is left is for bors to build and merge this PR. (Remember you need to say `bors r+`.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants