-
Notifications
You must be signed in to change notification settings - Fork 63
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAttention:
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. |
18db7d5
to
d97cad0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
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. |
Well then, back to brackets and no errors. (Since errors also tend to break things.) We can discuss this further in #1492. |
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. |
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.
I merged latest master into this and applied some fixes to docstrings. |
Co-authored-by: Max Horn <[email protected]>
test/generic/MPoly-test.jl
Outdated
|
||
ZZxxx0_ = polynomial_ring(ZZ, :x=>Base.OneTo(3)) | ||
ZZxxx_ = polynomial_ring(ZZ, :x=>1:3) | ||
ZZxxx2, (xxx2,) = ZZ[:x=>1:3] |
There was a problem hiding this comment.
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 ....
Implementation for oscar-system/Oscar.jl#2251.
Summary of the new features:
variables for
polynomial_ring
could always be specified in blocks, each block either aVector{<: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 aMatrix{<:Varname}
or indeed anyAbstractArray{<:VarName}
.For example, the following two lines both define
R
and setx
to a vector,y
to a matrix andz
to a single value.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 setx1
,x2
,x3
as well asy11
,y21
,y12
,y22
andz
to the corresponding generators ofR
.Of course this also works with zero-based ranges. E.g. the following introduces variables
x0
,x1
andx2
into the current scope.All of these features are implemented via a new macro
@varnames_interface
. This allows us to also support these variable name features infree_associative_algebra
,power_series_ring
, etc. See last could lines of the new fileVarNames.jl
to see where we applied it. And of course e.g. Oscar can also use it forfree_group
etc.Todos
polynomial_ring
again → Fixes documentation failures@varnames_interface
in (developer) documentation1:n
as alias forn
when giving dimensionsgraded_polynomial_ring
andpower_series_ring
which contain an optional weights argument