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

setindex! for Fac #2012

Open
SoongNoonien opened this issue Feb 25, 2025 · 5 comments
Open

setindex! for Fac #2012

SoongNoonien opened this issue Feb 25, 2025 · 5 comments

Comments

@SoongNoonien
Copy link
Contributor

I'm a bit confused by the behavior of setindex! for Fac. Currently I see no advantage over mulpow!. I'd expect to be able to change the exponents of factors already included, especially considering the docstring:

If $b$ is a factor of $a$, the corresponding entry is set to $c$.

What's also a bit weird, is that with setindex! non-positive exponents can be set. Resulting in interesting outputs:

julia> fac=factor(q^2-1)
1 * (q - 1) * (q + 1)

julia> setindex!(fac, 0, q);

julia> fac
1 * (q - 1) * q^0 * (q + 1)

julia> setindex!(fac, -1, q-2);

julia> fac
1 * (q - 1) * (q - 2)^-1 * q^0 * (q + 1)

julia> evaluate(fac)
ERROR: DomainError with -1:
Exponent must be non-negative
Stacktrace:
 [1] ^(x::QQPolyRingElem, y::Int64)
   @ Nemo ~/.julia/packages/Nemo/y7UoI/src/flint/fmpq_poly.jl:166
 [2] evaluate(a::Fac{QQPolyRingElem})
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/HxIHB/src/Factor.jl:56
 [3] top-level scope
   @ REPL[67]:1


@thofma
Copy link
Member

thofma commented Feb 25, 2025

In Hecke:

julia> factor(ZZ, QQ(1//100))
1 * 5^-2 * 2^-2

Regarding the setindex! and mulpow!. Yes, there is some overlapping functionality, but it is quite "natural" to have it given the other functionality for Fac.

@SoongNoonien
Copy link
Contributor Author

Ok, but zero as exponent should never be useful, except setting zero removes the factor all together. And why is redefining of exponents not allowed?

@thofma
Copy link
Member

thofma commented Feb 25, 2025

Because someone thought it was a good idea when it was implemented. But I guess we could change it, if there is some interest.

@fieker
Copy link
Contributor

fieker commented Feb 26, 2025

the way it was used orignally: redefining an exponent meant a logic flaw in the code...

@fingolfin
Copy link
Member

I understand this desire to prevent accidental changes to a Fac struct, but in that case wouldn't it make even more sense to disallow setindex! completely? To stop users from accidentally modifying the Fac at all?

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

No branches or pull requests

4 participants