Skip to content

Revert exponent rationalization efforts #81

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
merged 2 commits into from
Jul 8, 2024

Conversation

SoongNoonien
Copy link
Member

@SoongNoonien SoongNoonien commented Jul 5, 2024

I was a bit quick with those. While converting the 2B2 table locally to rational exponents I noticed many other problems that prevented importing. Finally I could resolve all of these just to notice that the whole concept is flawed. Take a look at nesum where it is mathematically necessary to have a well defined multiplication of modulusring elements into the argumentring. We could work around these by using the mapping between q and q0 but I think this makes things way to complicated.
My new idea is now to fully integrate the congruence field of the tables into the simplification of the cyclotomics. All tables with nonrational expressions in their character values have a congruence which ensures rationality in the exponents. This way we can use something like change_base_ring to ensure rationality while applying the normal form algorithms.

@fingolfin I'm not sure if this is how you like reverts to be done but I didn't want to force push to master.

@SoongNoonien SoongNoonien requested a review from fingolfin July 5, 2024 08:14
Copy link

codecov bot commented Jul 5, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.37%. Comparing base (b4e11a4) to head (8707ee8).

Files Patch % Lines
src/Arith.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
- Coverage   70.45%   70.37%   -0.08%     
==========================================
  Files          10       10              
  Lines         907      908       +1     
==========================================
  Hits          639      639              
- Misses        268      269       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SoongNoonien SoongNoonien linked an issue Jul 5, 2024 that may be closed by this pull request
@fingolfin fingolfin merged commit 0ad3ecc into oscar-system:master Jul 8, 2024
6 of 8 checks passed
@fingolfin
Copy link
Member

OK, let's talk more about it next week (I'll be back in KL only on Thursday and that day is already "full")

@SoongNoonien SoongNoonien deleted the revert branch August 14, 2024 09:15
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.

Get rid of number field elements in exponents?
2 participants