Skip to content

[Merged by Bors] - fix: fix Q-smul diamond in Complex#5341

Closed
ChrisHughes24 wants to merge 13 commits intomasterfrom
qsmul-diamond
Closed

[Merged by Bors] - fix: fix Q-smul diamond in Complex#5341
ChrisHughes24 wants to merge 13 commits intomasterfrom
qsmul-diamond

Conversation

@ChrisHughes24
Copy link
Copy Markdown
Member

@ChrisHughes24 ChrisHughes24 commented Jun 21, 2023


The example that has now been added to Data.Complex.Module did not commute before, but now that the definition of qsmul in the Field instance for complex numbers has been changed, it does commute.

I also fixed the statement of rat_cast_re, so it's an equality of real numbers now, without a coercion to complex. I also restored the norm_cast attribute on rat_cast_im and rat_cast_re since there is no error, probably because the coercion to Complex has been removed.

Open in Gitpod


noncomputable instance : Field ℂ :=
{ inv := Inv.inv
{ qsmul := fun n z => ⟨n • z.re - 0 * z.im, n • z.im + 0 * z.re⟩
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a note here why we have the weird definition?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a comment, but I actually don't understand the comment to do with RestrictScalars on the instance in Data.Complex.Module, so I don't fully understand the reason behind this change.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I should maybe improve that comment as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just realised I could also change the definition of Field to this, and change the ratCast def to go through the reals. This also makes the instance diagram commute, but for a more complex reason.

noncomputable instance : Field ℂ :=
{ ratCast := fun q => (q : ℝ)
  ratCast_mk := fun a b h1 h2 => by
    show ofReal' _ = _
    simp [Rat.cast_mk' a b h1 h2]; rfl
  inv := Inv.inv
  mul_inv_cancel := @Complex.mul_inv_cancel
  inv_zero := Complex.inv_zero }

@j-loreaux j-loreaux requested a review from eric-wieser June 21, 2023 17:38
@j-loreaux
Copy link
Copy Markdown
Contributor

@eric-wieser I know you're not a fan of the weird definition of docs#Module.complexToReal for nice defeqs, so I thought you may want to see this.

@j-loreaux j-loreaux changed the title fix(Data/Complex/Basic): fix Q-smul diamond fix: fix Q-smul diamond in Complex Jun 21, 2023
@kim-em kim-em added awaiting-author A reviewer has asked the author a question or requested changes. merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) and removed awaiting-review labels Jun 22, 2023
@ChrisHughes24 ChrisHughes24 added awaiting-review and removed awaiting-author A reviewer has asked the author a question or requested changes. merge-conflict The PR has a merge conflict with master, and needs manual merging. (this label is managed by a bot) labels Jun 22, 2023
ChrisHughes24 and others added 3 commits June 22, 2023 14:01
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
@kim-em
Copy link
Copy Markdown
Contributor

kim-em commented Jun 23, 2023

bors merge

@github-actions github-actions bot added ready-to-merge This PR has been sent to bors. and removed awaiting-review labels Jun 23, 2023
bors bot pushed a commit that referenced this pull request Jun 23, 2023
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
@bors
Copy link
Copy Markdown

bors bot commented Jun 23, 2023

Pull request successfully merged into master.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title fix: fix Q-smul diamond in Complex [Merged by Bors] - fix: fix Q-smul diamond in Complex Jun 23, 2023
@bors bors bot closed this Jun 23, 2023
@bors bors bot deleted the qsmul-diamond branch June 23, 2023 07:23
kim-em pushed a commit that referenced this pull request Jun 25, 2023
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
kbuzzard pushed a commit that referenced this pull request Jul 6, 2023
Co-authored-by: Chris Hughes <33847686+ChrisHughes24@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge This PR has been sent to bors.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants