Closed wbhart closed 5 years ago
Are we all happy with this? @fieker
Now that we have map we don't need change_base_ring with arbitrary callable argument. If the second argument is another ring, it is not a function so the convention does not apply. So one could argue that change_base_ring could keep the old order.
I personally don't care. Both options are fine for me.
PS: If we want to do this, we should do it quickly.
Talked to Claus. Let's make the ring the first argument.
Where do we put the additional target polynomial ring for polynomials?
change_base_ring(R::Ring, f::PolyElem, target_polyring::PolyRing)
Like this?
I'm not really sure. Does @rfourquet have an opinion? He is probably most familiar with these conventions.
By the way, I didn't make a comprehensive list of such functions. There could be more hiding in there somewhere. I'll have a look through the documentation and see if I can spot any others.
There's also an evaluate for multivariates that takes a map or ring argument as the last argument.
I don't know if lu and fflu should have their permgroup first?
There's also map_with_retraction_from_func and map_with_section_from_func
That's all I can find in Nemo and AbstractAlgebra. The one other thing that could eventually be affected is the special functions in Nemo (for creating various kinds of special polynomials/series). At present they take a second argument which is a variable. The idea was that we'd eventually accept a polynomial as argument and return the function evaluated at that polynomial. But I think it would be better to just accept a polynomial ring as argument and let people use evaluation/substitution to get whatever else they wanted. I don't think we should try to provide too much combined functionality, but rather orthogonal things that get people where they need to go.
Wait, are you really proposing removing the ability to specify a Ring, Map or Julia function for the "Ring" argument? I think that is a bad idea, on performance grounds.
Or are you saying that map precisely duplicates some functionality here?
Yes, duplicated functionality and the name change_base_ring
does not make sense for arbitrary maps, i.e., change_base_ring(x + 1, z -> z + 1)
.
Ok. Can you summarise what kinds of arguments change_base_ring will accept, as we need to make the same change in Singular.jl.
I made the suggestion to use
change_base_ring(R::Ring, f::PolyElem, [R::PolyRing])
where the last argument is the optional target polynomial ring.
The map version of this would look like
map(C::anything that is callable, f::PolyElem, [R::PolyRing])
where again the last argument is the optional target polynomial ring.
Yeah that sounds ok, so long as it is extremely clearly described in the documentation. At first I was quite confused about the optional polynomial ring.
There are currently a lot of doctest failures due to the changes to change_base_ring.
Hmm, I see them.
I think I remember now, even if the doctests fail, the documentation build won't error ...
Once the dust settles, we should do https://juliadocs.github.io/Documenter.jl/stable/man/doctests/#Fixing-Outdated-Doctests-1
This might explain my documentation bug that I mentioned by email. If the docs error out, maybe they are not deployed.
Yes, also docs are never deployed from branches other than master or pull requests.
That is the desired behaviour. Of course only a merged PR should update the docs.
I'm not really sure. Does @rfourquet have an opinion? He is probably most familiar with these conventions.
Sorry to answer late after @thoma already made a PR, but maybe the signature should be:
change_base_ring(R::Ring, [R::PolyRing], f::PolyElem)
.
According to the style guide, types would come after the the function argument:
Passing a type typically means that the output will have the given type
Here we don't have a type, but a PolyRing
which specifies the target parent object (which somehow play the role of a type).
I agree, this is what the style guide would suggest in this instance.
Ok, will get to it on Monday.
@raulepure mentioned that the third ring argument is optional and it seems odd to have an optional argument as the second argument of three.
@rfourquet Does Julia have a convention on that?
it seems odd to have an optional argument as the second argument of three.
No there is no Julia convention on that, and, although slightly unconvenient to implement because you can not use default argument with Julia syntax (you have to write 2 methods yourself), it's pretty common. For example rand(2, 3)
is equivalent to rand(Float64, 2, 3)
, or collect(1:9)
is equivalent to collect(eltype(1:9), 1:9)
.
Yes, I have also seen this in Base. It is not too uncommon.
Unless someone beats me to it, I will get to it on Monday. There is also the corresponding function for univariate polynomials which I oversaw.
As noted by @rfourquet in https://github.com/Nemocas/AbstractAlgebra.jl/pull/431#discussion_r329496414, the map
syntax for multivariate polynomials has some problems if one also supplies the target polynomial ring.
The reason is that the Base.map
function does the following
julia> map((x, y) -> x + y, 1, 2)
3
In our case a call to map(z -> z^2, Rx, f)
would only map z -> z^2
over the coefficients of f
and return something in Rx
. So this is quite different to what map
does with multiple arguments. This is a bit of a shame, since I like map
and thinking of polynomials as collections.
So I see three options:
change_base_ring
, e.g., change_base_ring(z -> z^2, f)
or change_base_ring(z -> z^2, Rx, f)
(with target polynomial ring).map
in Base.map(z -> z^2, Rx, f)
or map(z -> z^2, f)
.My opinion:
change_base_ring
.or a keyword argument for the ring? On Mon, Sep 30, 2019 at 03:55:46AM -0700, thofma wrote:
As noted by @rfourquet in https://github.com/Nemocas/AbstractAlgebra.jl/pull/431#discussion_r329496414, the
map
syntax for multivariate polynomials has some problems if one also supplies the target polynomial ring.The reason is that the
Base.map
function does the followingjulia> map((x, y) -> x + y, 1, 2) 3
In our case a call to
map(z -> z^2, Rx, f)
would only mapz -> z^2
over the coefficients off
and return something inRx
. So this is quite different to whatmap
does with multiple arguments. This is a bit of a shame, since I likemap
and thinking of polynomials as collections.So I see three options:
- Remove map for polynomials and use only
change_base_ring
, e.g.,change_base_ring(z -> z^2, f)
orchange_base_ring(z -> z^2, Rx, f)
(with target polynomial ring).- Ignore the concerns about the semantics of
map
in Base.- Think of a new name for
map(z -> z^2, Rx, f)
ormap(z -> z^2, f)
.My opinion:
- It is weird to call this
change_base_ring
.- Would rather not do it.
- Yes, but which name?
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/421#issuecomment-536510466
julia> map(x -> x + 1, 1, a = 2)
ERROR: function map does not accept keyword arguments
Yes, so keyword argument would work. So it would be
map(z -> z^2, f, polyring = Rx)
or any other name for the keyword argument.
I don't have a strong opinion, but I lean slightly towards not overloading map, for the reasons @rfourquet suggested.
So is the problem here that change_base_ring should only change the base ring and nothing else and therefore passing an optional polynomial ring is not in the spirit of change_base_ring?
I'm not totally convinced by this argument as it seems like the main application of this extra argument will be to make it possible to implement change_base_ring the way we want, e.g. in Singular we would certainly use the optional argument as we have a slightly more complicated situation there.
I also prefer to have the optional argument in the third position. I mean, isn't it the case that you would always have to call it as change_base_ring(R, , p) if you want to take advantage of the optional argument? That is just ugly.
A keyword argument is also fine for me.
Regarding the position of the addtional polynomial ring argument: It will still be change_base_ring(R, p)
. The full version would be change_base_ring(R, Rx, p)
(or change_base_ring(R, p, polyring = Rx)
if we go for the keyword argument).
Regarding the use of map
or just using change_base_ring
for both: My problem with 1) is that change_base_ring(z -> z^2, f)
is not changing the base ring. There is a mismatch between the name of the function ("change_base_ring") and what it is actually doing ("applying a map coefficientwise"). This is my only concern with 1).
- It is weird to call this change_base_ring
I'm not a fan of this name, but for now it exists already for other objects, so as long as we have it, we might as well use this name consistently in all cases. Otherwise you may face each time the hesitation of using change_base_ring
vs map
.
- Ignore the concerns about the semantics of map in Base.
I agree we shouldn't do that. Even for the simple reason that we might want it. It's not implemented yet, but using multiple arguments with map
could become useful.
Think of a new name for map(z -> z^2, Rx, f) or map(z -> z^2, f).
I'm not sure if considering mutable polynomials is fine, but typically for e.g. matrices, if you want to have control over the output type, you create the output object yourself and pass it as first argument to map!
, which will update it.
There is a mismatch between the name of the function ("change_base_ring") and what it is actually doing
Oh I missed this part, in my previous answer. Good point.
Renaming change_base_ring
to map_base_ring
comes to mind, but it's kind of ugly.
On Mon, Sep 30, 2019 at 04:18:36AM -0700, wbhart wrote:
I don't have a strong opinion, but I lean slightly towards not overloading map, for the reasons @rfourquet suggested.
So is the problem here that change_base_ring should only change the base ring and nothing else and therefore passing an optional polynomial ring is not in the spirit of change_base_ring? This is neccessary if you want to have change_base_ring(m, f) + change_base_ring(m, g) to work - provided f+g used to work Passing the poly ring was added to make sure that change_base_ring lands in the exact same ring. We do not want to start requireing/ using cached=true/false in change_base_ring...
I'm not totally convinced by this argument as it seems like the main application of this extra argument will be to make it possible to implement change_base_ring the way we want, e.g. in Singular we would certainly use the optional argument as we have a slightly more complicated situation there.
I also prefer to have the optional argument in the third position. I mean, isn't it the case that you would always have to call it as change_base_ring(R, , p) if you want to take advantage of the optional argument? That is just ugly.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/421#issuecomment-536517341
I'm not keen on changing the name. I really prefer to make it a keyword argument. It's just some technical functionality required to make stuff work. I don't think we should worry about applying any conventions, or worry about the fact that it could be abused to do something that isn't intended by the name.
So @fieker, @thofma and I had a discussion on this, and we prefer map_coefficients (with an abbreviation map_coeffs also available) and map_entries for matrices. This gets us out of the namespace of Julia which could end up biting us if they decide to make map apply to everything that is not a collection (which is a decision they have commonly made elsewhere).
change_base_ring will just take a ring and not allow abstract algebra Maps or functions.
Does this address all your concerns @rfourquet ?
I can see you probably wanted map to apply to our matrices, so that they are more familiar to Julia users, as Julia abuses notation and considers matrices to be collections. But I think this is not wise, as we cannot (and won't) implement the entire AbstractArray interface for our matrices. So it's not a good idea to pretend that it does. Also, fundamentally, our matrices are more than just collections.
if they decide to make map apply to everything that is not a collection
I don't understand, could you give an example? (are you referring to e.g. broadcasting?)
In a post above, I started proposing exactly this, map_coeffs
, but removed it immediatly as I thought it's not a generic name, so another name will be needed for other rings. Conversely, change_base_ring
is generic, with unambiguous meaning, because base_ring
is well understood in the AA-verse.
If we want to apply a function which doesn't necessarily change the ring, my personal preference is to have similarly a generic name for all the rings, e.g. map_base_ring
. Also personally I would choose to define only one function, not two, which may or may not change the base ring, and call it map_base_ring
(or whatever better name) which can accept a ring or a function/AA-Maps as argument.
I can see you probably wanted map to apply to our matrices
Yes, but I implemented it because it was a project for the Oscar meeting in June :)
But indeed, I agree that Julia abuses notation, and I find it even worse for Vector
, but this won't change. On our side, we interract a lot with Julia matrices for e.g. constructors, so we can't deny that Julia and AA matrice are cousins in a way, and I would favor supporting the APIs which can be expected by new users, make their life simpler, and doesn't cost us anything. For example, I wish that AA-matrices have iteration, so that I can do e.g. collect(AAmat)
and send that to a LinearAlgebra
algorithm which requires an AbstractMatrix = AbstractArray{T,2}
.
Also, fundamentally, our matrices are more than just collections.
As are Julia matrices. Collections don't have to be reduced to a datastructure which is only about storing elements, they can have a meaning by themselves. I'm not able to articulate exactly why, but I feel it's fine to consider matrices as collection, unlike polynomials, where a coefficient is associated to an exponent (i.e. I would consider more acceptable to consider a polynomial as a collection of pairs coeff => exponent
).
if they decide to make map apply to everything that is not a collection
I don't understand, could you give an example? (are you referring to e.g. broadcasting?)
I was thinking of something we saw the other day that used to only be defined for matrices (where it makes sense), but they now define it for Integers (where it does not) because that is convenient.
But I forgot what example that is.
In a post above, I started proposing exactly this,
map_coeffs
, but removed it immediatly as I thought it's not a generic name, so another name will be needed for other rings. Conversely,change_base_ring
is generic, with unambiguous meaning, becausebase_ring
is well understood in the AA-verse.If we want to apply a function which doesn't necessarily change the ring, my personal preference is to have similarly a generic name for all the rings, e.g.
map_base_ring
. Also personally I would choose to define only one function, not two, which may or may not change the base ring, and call itmap_base_ring
(or whatever better name) which can accept a ring or a function/AA-Maps as argument.
I would normally agree, but for multivariates you are going to have:
They can't all be the same name! This will only get more complicated as we implement more complex mathematical objects where there are lots of things you want to apply maps to.
I suggested we drop it altogether and let the user write code for applying a map to coefficients, but @fieker and @thofma don't want that (I'm also not that keen on it either).
We decided on the rule that the name should be the same as the iterator, i.e. if we have coefficients as the iterator for the coefficients of a multivariate, then map_coefficients should be the function that applies a map to the coefficients.
I can see you probably wanted map to apply to our matrices
Yes, but I implemented it because it was a project for the Oscar meeting in June :) But indeed, I agree that Julia abuses notation, and I find it even worse for
Vector
, but this won't change. On our side, we interract a lot with Julia matrices for e.g. constructors, so we can't deny that Julia and AA matrice are cousins in a way, and I would favor supporting the APIs which can be expected by new users, make their life simpler, and doesn't cost us anything. For example, I wish that AA-matrices have iteration, so that I can do e.g.collect(AAmat)
and send that to aLinearAlgebra
algorithm which requires anAbstractMatrix = AbstractArray{T,2}
.
I think we should supply iterators for as many objects as is practical. But it's not a good idea to support Julia conventions that aren't going to work well for us. And this one simply won't.
Also, fundamentally, our matrices are more than just collections.
As are Julia matrices. Collections don't have to be reduced to a datastructure which is only about storing elements, they can have a meaning by themselves. I'm not able to articulate exactly why, but I feel it's fine to consider matrices as collection, unlike polynomials, where a coefficient is associated to an exponent (i.e. I would consider more acceptable to consider a polynomial as a collection of pairs
coeff => exponent
).
I partially agree, but we have to make a decision which everyone can live with.
Sure, in this case, with coefficients, exponents, etc, different names seem to be useful! Still for the base ring a similarly named function could be used, but I won't insist. Also, do we want to rename change_base_ring
to change_coefficient_ring
, rename_entries_ring`, etc?
And this one simply won't.
Are you speaking about iteration, or map
? I don't see how it won't work in any case (for matrices), but I won't insist.
I think we like change_base_ring because that is what is used elsewhere (Magma). I also quite like it.
As base_ring is consistent across all our ring based objects, I don't think this will be problematic. If there is some other thing we want to change about an object we can name it after whatever field that is.
It's true, it's not consistent in one way, but it is in another. And there is certainly no hope whatsoever of making all things consistent in precisely one way.
I think the process of "making things consistent" doesn't converge in general.
I think the reason Julia can get away with just one name (map) is that they only apply it to collections, and those contain precisely one kind of thing. I don't think we should try to apply map to polynomials that contain more than one thing (coefficients, exponents, symbols, etc.)
I'm still very sceptical that it is a good idea to apply it to our matrices (and I don't think Julia should have done this either; it should only apply to arrays). But I wouldn't oppose having map_entries for matrices and map (both of which would do the same thing in the case of matrices). But I really believe it is a bad idea for polynomials.
Consider sparse polynomials for example. They are no longer just bags of coefficients any more, but have some structure to them. So they really aren't collections in any ordinary sense of the word.
Ah ok, yes I agree and believe too that map
on poiynomials is a bad idea, and was also precisely thinking of sparse polynomials. For predictible genericity, a non-qualifed map
should ignore the sparsity and apply the function to every coefficient, even those not stored. In the same way that map(f, m)
will "de-sparsify" a sparse matrix m
:
julia> m = sprand(Int8, 3, 4, .3)
3×4 SparseMatrixCSC{Int8,Int64} with 3 stored entries:
[2, 1] = 81
[2, 2] = -127
[2, 3] = -109
julia> map(x->x+1, m)
3×4 SparseMatrixCSC{Int64,Int64} with 12 stored entries:
[1, 1] = 1
[2, 1] = 82
[3, 1] = 1
[1, 2] = 1
[2, 2] = -126
[3, 2] = 1
[1, 3] = 1
[2, 3] = -108
[3, 3] = 1
[1, 4] = 1
[2, 4] = 1
[3, 4] = 1
I like the idea to have both map_entries
and map
as an alias. I personally don't believe it was a bad idea to have map
on Matrix
in Julia, but this won't change anyway. But replicating some functionalities seems to make sense when there is no associated cost.
Ah ok, yes I agree and believe too that
map
on poiynomials is a bad idea, and was also precisely thinking of sparse polynomials. For predictible genericity, a non-qualifedmap
should ignore the sparsity and apply the function to every coefficient, even those not stored.
I look forward to seeing your implementation of that for x^1000000y^3000000z^10000000.
Sorry, but this just won't work!
In the same way that
map(f, m)
will "de-sparsify" a sparse matrixm
:
That's an incredibly bad idea!
julia> m = sprand(Int8, 3, 4, .3) 3×4 SparseMatrixCSC{Int8,Int64} with 3 stored entries: [2, 1] = 81 [2, 2] = -127 [2, 3] = -109 julia> map(x->x+1, m) 3×4 SparseMatrixCSC{Int64,Int64} with 12 stored entries: [1, 1] = 1 [2, 1] = 82 [3, 1] = 1 [1, 2] = 1 [2, 2] = -126 [3, 2] = 1 [1, 3] = 1 [2, 3] = -108 [3, 3] = 1 [1, 4] = 1 [2, 4] = 1 [3, 4] = 1
I like the idea to have both
map_entries
andmap
as an alias. I personally don't believe it was a bad idea to havemap
onMatrix
in Julia, but this won't change anyway. But replicating some functionalities seems to make sense when there is no associated cost.
Yep.
Sorry, but this just won't work!
I certainly won't implement that, as I said I don't like to define map
on polynomials. Just on matrices, as they can be seen unambiguously as collections.
Defining map(f, m)
on a sparse matrix the way it is in Base
may not be efficient, but is the only sensible way to do it at all. If the non-stored entries are skipped, then it can't be called map
.
I suppose it's ok if they first check if 0 maps to 0. If so, then they never need to create a dense matrix. Otherwise, it is totally idiotic. I'm not sure how that would respect type stability though.
On Mon, Sep 30, 2019 at 11:09:17AM -0700, wbhart wrote:
Ah ok, yes I agree and believe too that
map
on poiynomials is a bad idea, and was also precisely thinking of sparse polynomials. For predictible genericity, a non-qualifedmap
should ignore the sparsity and apply the function to every coefficient, even those not stored.I look forward to seeing your implementation of that for x^1000000y^3000000z^10000000.
Sorry, but this just won't work!
In the same way that
map(f, m)
will "de-sparsify" a sparse matrixm
:That's an incredibly bad idea! This is automatic. Note: the type is stable here, its just that the function may not be a homomorphism, thus not mapping zero to zero. If you want, try f(0) ONCE and then either ignore the zero entries or curse the stupid user.
julia> m = sprand(Int8, 3, 4, .3) 3×4 SparseMatrixCSC{Int8,Int64} with 3 stored entries: [2, 1] = 81 [2, 2] = -127 [2, 3] = -109 julia> map(x->x+1, m) 3×4 SparseMatrixCSC{Int64,Int64} with 12 stored entries: [1, 1] = 1 [2, 1] = 82 [3, 1] = 1 [1, 2] = 1 [2, 2] = -126 [3, 2] = 1 [1, 3] = 1 [2, 3] = -108 [3, 3] = 1 [1, 4] = 1 [2, 4] = 1 [3, 4] = 1
I like the idea to have both
map_entries
andmap
as an alias. I personally don't believe it was a bad idea to havemap
onMatrix
in Julia, but this won't change anyway. But replicating some functionalities seems to make sense when there is no associated cost.Yep.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/421#issuecomment-536682562
On Mon, Sep 30, 2019 at 08:33:44AM -0700, wbhart wrote:
if they decide to make map apply to everything that is not a collection
I don't understand, could you give an example? (are you referring to e.g. broadcasting?)
I was thinking of something we saw the other day that used to only be defined for matrices (where it makes sense), but they now define it for Integers (where it does not) because that is convenient.
But I forgot what example that is.
In a post above, I started proposing exactly this,
map_coeffs
, but removed it immediatly as I thought it's not a generic name, so another name will be needed for other rings. Conversely,change_base_ring
is generic, with unambiguous meaning, becausebase_ring
is well understood in the AA-verse.If we want to apply a function which doesn't necessarily change the ring, my personal preference is to have similarly a generic name for all the rings, e.g.
map_base_ring
. Also personally I would choose to define only one function, not two, which may or may not change the base ring, and call itmap_base_ring
(or whatever better name) which can accept a ring or a function/AA-Maps as argument.I would normally agree, but for multivariates you are going to have: Why?
- map_coefficients Yep, used that
- map_exponent_vectors no, never missed this - although I might infer a use case
- map_monomials no point.
They can't all be the same name! This will only get more complicated as we implement more complex mathematical objects where there are lots of things you want to apply maps to. Thats why, in general, I suggested a induce_map, induce_image API...
I suggested we drop it altogether and let the user write code for applying a map to coefficients, but @fieker and @thofma don't want that (I'm also not that keen on it either). It's too frequent to have users write this everytime it occurs.
We decided on the rule that the name should be the same as the iterator, i.e. if we have coefficients as the iterator for the coefficients of a multivariate, then map_coefficients should be the function that applies a map to the coefficients.
I can see you probably wanted map to apply to our matrices
Yes, but I implemented it because it was a project for the Oscar meeting in June :) But indeed, I agree that Julia abuses notation, and I find it even worse for
Vector
, but this won't change. On our side, we interract a lot with Julia matrices for e.g. constructors, so we can't deny that Julia and AA matrice are cousins in a way, and I would favor supporting the APIs which can be expected by new users, make their life simpler, and doesn't cost us anything. For example, I wish that AA-matrices have iteration, so that I can do e.g.collect(AAmat)
and send that to aLinearAlgebra
algorithm which requires anAbstractMatrix = AbstractArray{T,2}
.I think we should supply iterators for as many objects as is practical. But it's not a good idea to support Julia conventions that aren't going to work well for us. And this one simply won't.
Also, fundamentally, our matrices are more than just collections.
As are Julia matrices. Collections don't have to be reduced to a datastructure which is only about storing elements, they can have a meaning by themselves. I'm not able to articulate exactly why, but I feel it's fine to consider matrices as collection, unlike polynomials, where a coefficient is associated to an exponent (i.e. I would consider more acceptable to consider a polynomial as a collection of pairs
coeff => exponent
).I partially agree, but we have to make a decision which everyone can live with.
-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/Nemocas/AbstractAlgebra.jl/issues/421#issuecomment-536618150
I suppose it's ok if they first check if 0 maps to 0. If so, then they never need to create a dense matrix.
Yes if 0 maps to 0, then the sparsity remains the same. I don't know the implementation, but it seems it's even checked only once, so no time is lost repeatedly calling f(0)
on each and every non-stored entry.
Who cares if those other functions are ever needed or implemented. The point is, a polynomial is not just a collection and you may wish to map more than coefficients, just as in other mathematical objects you may wish to map more than just one part of the object.
It's a general principal that polynomials are not suitable for map. The latter should only be used for genuine collection types (which on principle, all mathematical types are not).
See edict 1 here:
https://docs.julialang.org/en/v1/manual/style-guide/index.html#Write-functions-with-argument-ordering-similar-to-Julia-Base-1
(assuming the documentation and convention doesn't change before you look at it).