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

Wrap Calcium fields/numbers #1077

Merged
merged 49 commits into from
Jun 3, 2021
Merged

Conversation

fredrik-johansson
Copy link
Contributor

Follows #1070.

This adds CalciumField and the ca type.

Work in progress; has most of the functionality (sans ca_poly and ca_mat) but needs docs and tests.

@fredrik-johansson
Copy link
Contributor Author

Manual reference counting seems to be working.

@fredrik-johansson
Copy link
Contributor Author

Any ideas why [matching: [Oscar#master,Hecke#master,Singular#master] - ubuntu-latest, julia ~1.6.0-0] is stuck in "Run tests" for two hours with no output at all?

@thofma
Copy link
Member

thofma commented May 31, 2021

I have restarted it.

@fredrik-johansson
Copy link
Contributor Author

@thofma Do you have a workaround for JuliaDocs/Documenter.jl#558 ?

I want to document e.g. (R::AcbField)(a::ca; parts::Bool=false).

Or should I make a named function for this?

@thofma
Copy link
Member

thofma commented May 31, 2021

No, there is no work around as far as I know. We usually handcraft the "constructor" section like here: https://nemocas.github.io/AbstractAlgebra.jl/latest/polynomial_rings/#Constructors.

@fredrik-johansson
Copy link
Contributor Author

This is ready for review!

@fredrik-johansson
Copy link
Contributor Author

The rand function is a placeholder, by the way. I can open a separate issue to discuss what kind of options we want here.

Copy link
Member

@thofma thofma left a comment

Choose a reason for hiding this comment

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

In the docstring and in the docs, you are using *a* quite a alot. I think all of these should be `a`? I assume they should be formated as code and not italicized?


Return the imaginary unit $i$ as an element of *C*.
"""
function const_i(C::CalciumField)
Copy link
Member

Choose a reason for hiding this comment

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

For AcbField this is called onei.

Nothing, (Ref{ca}, Ref{CalciumField}), a, a.parent)
a.parent.refcount -= 1
if a.parent.refcount == 0
ccall((:ca_ctx_clear, libcalcium), Nothing, (Ref{CalciumField},), a.parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

Codecov seems to indicate ca_ctx_clear is never called. Could it be the refcounting is not working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is precisely the path that only gets called if Julia calls finalizers out of order, which only happens on some versions and on some machines. However, introducing a helper function should solve the coverage issue.


elem_type(::Type{CalciumField}) = ca

base_ring(a::CalciumField) = Union{} # ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these are correct. No need for the ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I copied this from another module and I was just not sure about the meaning.

#
###############################################################################

export ca, CalciumField, infinity, unsigned_infinity, undefined, unknown,
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical order please. I know there are no other good examples of this.

#
###############################################################################

# todo: implement nontrivial hash functions on C
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this await something on the C side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's a nontrivial function to implement (well).

return r
end

function +(a::ca, b::fmpz)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have generic code that switches the order of arguments if one ad hoc operation is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea what's happening behind the scenes, but the reverse operation works and is tested. I will add the reverse functions anyway to make sure it's fast.

check_special(r)
return r
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you separate into sections Ad hoc division, Powering, Unary operations, for conformity with the other files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

return r
end

###############################################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually put the predicates before the arithmetic ops, usually in a section called Basic manipulation.

end

function mul!(z::ca, x::ca, y::ca)
if z.parent != x.parent || x.parent != y.parent
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not check parents in unsafe operators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nonetheless, this check did help catch a bug. I'd rather keep it until the code is more battle-tested.

@@ -770,7 +782,7 @@ end
Return the extension of the real sign function taking the value 1
strictly in the right half plane, -1 strictly in the left half plane,
and the sign of the imaginary part when on the imaginary axis.
Equivalently, $\csgn(x) = x / \sqrt{x^2}$ except that the value is 0
Equivalently, $\operatorname{csgn}(x) = x / \sqrt{x^2}$ except that the value is 0
Copy link
Contributor

Choose a reason for hiding this comment

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

is \operatorname standard latex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's the usual way to define a named operator.


end

@testset "ca.adhoc-operations" begin
Copy link
Contributor

Choose a reason for hiding this comment

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

underscores, not minus

@wbhart
Copy link
Contributor

wbhart commented Jun 2, 2021

Looks good overall. Just a few niggles.

@fredrik-johansson
Copy link
Contributor Author

In the docstring and in the docs, you are using *a* quite a alot. I think all of these should be `a`? I assume they should be formated as code and not italicized?

It's just more convenient to type. I will fix this for consistency.

@fredrik-johansson
Copy link
Contributor Author

@wbhart @thofma Niggles fixed, I think.

@thofma thofma merged commit d530e05 into Nemocas:master Jun 3, 2021
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.

3 participants