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

Add Base.conj for AbsSimpleNumFieldElem #1622

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Conversation

SoongNoonien
Copy link
Contributor

As discussed with @fieker, @lgoettgens and @joschmitt on Friday at the OSCAR workshop I've added a Base.conj method for AbsSimpleNumFieldElem. The main purpose of this is to provide an easy way to conjugate cyclotomic numbers interactively (for actual implementations complex_conjugation should be better). After looking at the code I'm not sure why checking the parent field should be necessary. I think we can leave all the checks to complex_conjugation.

Copy link

codecov bot commented Sep 21, 2024

Codecov Report

Attention: Patch coverage is 76.47059% with 8 lines in your changes missing coverage. Please review.

Project coverage is 75.87%. Comparing base (d9b7787) to head (c1c70c4).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/NumField/NfAbs/Conjugates.jl 76.47% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1622      +/-   ##
==========================================
+ Coverage   75.78%   75.87%   +0.08%     
==========================================
  Files         361      361              
  Lines      113695   113693       -2     
==========================================
+ Hits        86164    86261      +97     
+ Misses      27531    27432      -99     
Files with missing lines Coverage Δ
src/NumField/NfAbs/Conjugates.jl 83.98% <76.47%> (+0.09%) ⬆️

... and 38 files with indirect coverage changes

@thofma
Copy link
Owner

thofma commented Sep 22, 2024

I think it is reasonable to have this, thanks. How about we cache the result of complex_conjugation(K) using @attr? Then we can remove the !!! note.

@SoongNoonien
Copy link
Contributor Author

I'm not very familiar with this @attr. Do you mean something like this: @attr NumFieldHom{AbsSimpleNumField, AbsSimpleNumField, MapDataFromAnticNumberField{AbsSimpleNumFieldElem}, MapDataFromAnticNumberField{AbsSimpleNumFieldElem}, AbsSimpleNumFieldElem} function complex_conjugation(...?

@thofma
Copy link
Owner

thofma commented Sep 22, 2024

I noticed that we cannot use @attr, since it does not work with keyword arguments. I pushed a commit adding the caching and merge once everything is green.

@thofma thofma merged commit 69fb452 into thofma:master Sep 23, 2024
17 checks passed
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