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

Unified interface for constructors containing variable names #1360

Merged
merged 128 commits into from
Nov 22, 2023

Conversation

mgkurtz
Copy link
Contributor

@mgkurtz mgkurtz commented May 11, 2023

Implementation for oscar-system/Oscar.jl#2251.

Summary of the new features:

  1. variables for polynomial_ring could always be specified in blocks, each block either a Vector{<:VarName} or of the form "x" => 1:3 or "x" => (1:2,1:2) or even "x" => (1:2,1:2,3:5,-3:4). So with the second method, "matrix shaped" or "tensor shaped" blocks of variables could be introduced. To complement this, now you can also use a Matrix{<:Varname} or indeed any AbstractArray{<:VarName}.

    For example, the following two lines both define R and set x to a vector, y to a matrix and z to a single value.

    R, x, y, z = polynomial_ring(QQ, :x => 1:3, :y => (1:2, 1:2), :z)
    R, x, y, z = polynomial_ring(QQ, ["x[1]", "x[2]", "x[3]"], ["y[1,1]" "y[1,2]"; "y[2,1]" "y[2,2]"], :z)
  2. For interactive use, it can be annoying to have to repeat variable names. For this we have a new macro @polynomial_ring which automatically assigns matching Julia variables with the polynomial ring variables. This pairs well with the existing (but not widely known) method of writing "x#" => 1:3 as a shorthand for ["x1","x2",x"3].

    For example the next four lines all define R and set x1, x2, x3 as well as y11, y21, y12, y22 and z to the corresponding generators of R.

    R, x1, x2, x3, y11, y21, y12, y22, z = polynomial_ring(QQ, :x1, :x2, :x3, :y11, :y21, :y22, :y12, :y22, :z)
    R, (x1, x2, x3), (y11, y21, y12, y22), z = polynomial_ring(QQ, "x#" => 1:3, "y#" => (1:2, 1:2), :z)
    R = @polynomial_ring(QQ, :x1, :x2, :x3, :y11, :y21, :y22, :y12, :y22, :z)
    R = @polynomial_ring(QQ, "x#" => 1:3, "y#" => (1:2, 1:2), :z)

    Of course this also works with zero-based ranges. E.g. the following introduces variables x0, x1 and x2 into the current scope.

    R = @polynomial_ring(QQ, "x#" => 0:2)
    
  3. All of these features are implemented via a new macro @varnames_interface. This allows us to also support these variable name features in free_associative_algebra, power_series_ring, etc. See last could lines of the new file VarNames.jl to see where we applied it. And of course e.g. Oscar can also use it for free_group etc.

Todos

  • Fix method signatures
  • Allow key value arguments in macros
  • Remove manually added methods, which lead to method ambiguities and failing tests
  • Add docstrings for polynomial_ring again → Fixes documentation failures
  • Include @varnames_interface in (developer) documentation
  • Decide on which variants we want
  • Decide on what to do with counting from 0 on
  • Decide on whether we want 1:n as alias for n when giving dimensions
  • Decide on what to do with graded_polynomial_ring and power_series_ring which contain an optional weights argument
  • Think about how to split documentation up in user and developer documentation
  • Add references to the user documentation in all relevant constructor docstrings
  • Maybe add some more tests
  • Maybe look out for more constructors
  • Create PRs to use this machinery for constructors in Nemo, Hecke and Oscar

@fingolfin fingolfin requested review from thofma and fieker May 11, 2023 10:17
@thofma

This comment was marked as outdated.

@mgkurtz

This comment was marked as outdated.

@fieker

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (4b10ce0) 86.77% compared to head (181d615) 87.12%.
Report is 1 commits behind head on master.

Files Patch % Lines
src/MPoly.jl 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1360      +/-   ##
==========================================
+ Coverage   86.77%   87.12%   +0.34%     
==========================================
  Files         111      112       +1     
  Lines       28833    28807      -26     
==========================================
+ Hits        25020    25097      +77     
+ Misses       3813     3710     -103     

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

@mgkurtz mgkurtz force-pushed the mgk/varnames branch 2 times, most recently from 18db7d5 to d97cad0 Compare October 5, 2023 22:08
docs/src/constructors.md Outdated Show resolved Hide resolved
fingolfin

This comment was marked as outdated.

@fingolfin

This comment was marked as outdated.

docs/src/constructors.md Outdated Show resolved Hide resolved
docs/src/constructors.md Outdated Show resolved Hide resolved
docs/src/constructors.md Show resolved Hide resolved
@fingolfin
Copy link
Member

I chose, just to go with LATEX syntax for indices now and remove the warnings. So, x[1] may print as x_{-1} and x[2] as x_3. The warnings for non-identifier names like x-3 in macro mode (@polynomial_ring et al.) intended for interactive use remain.

Wasn't the agreement to not change this for now, to minimize the work that is needed to get this PR merged? With this change in place, now tons and tons of docstrings in all our projects will have to be updated when this PR is merged.

I'd really prefer to keep this kind of change out here, and introduce it in a separate PR, so it can be discussed on its own merits, without delaying this PR here further.

@mgkurtz
Copy link
Contributor Author

mgkurtz commented Nov 13, 2023

Well then, back to brackets and no errors. (Since errors also tend to break things.) We can discuss this further in #1492.

@mgkurtz
Copy link
Contributor Author

mgkurtz commented Nov 14, 2023

Now only invalidations and code coverage fail. The invalidations seem to be bad luck. The code coverage is due to some uncovered lines from the merged #1489.

@mgkurtz mgkurtz marked this pull request as ready for review November 15, 2023 09:35
fingolfin and others added 10 commits November 20, 2023 15:02
This function is type unstable, so at least internal code is better off not calling it
There is already a type assertion inside the code, so no
need for this conversion inducing type declaration.
- add `@attributes` mutable struct MPolyRing
- generic `iszero` for ideals
- Dan's code for gcd, factor for nested polynomial rings
Some time ago `with_unicode` was improved in Oscar, apply that here.
src/AbstractAlgebra.jl Outdated Show resolved Hide resolved
@fingolfin
Copy link
Member

I merged latest master into this and applied some fixes to docstrings.


ZZxxx0_ = polynomial_ring(ZZ, :x=>Base.OneTo(3))
ZZxxx_ = polynomial_ring(ZZ, :x=>1:3)
ZZxxx2, (xxx2,) = ZZ[:x=>1:3]
Copy link
Member

Choose a reason for hiding this comment

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

This is not allowed anymore in the latest iteration of this PR ....

@fingolfin fingolfin merged commit 562f826 into Nemocas:master Nov 22, 2023
14 of 15 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.

6 participants