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

Finite field sqrt #860

Closed
wants to merge 3 commits into from
Closed

Finite field sqrt #860

wants to merge 3 commits into from

Conversation

wbhart
Copy link
Contributor

@wbhart wbhart commented Jul 30, 2020

Note that this requires Flint-2.7 which will be released later this year, hence the breaking label. Tests will fail until then.

It also changes the actual finite field structs in line with Flint-2.7.

It adds sqrt and is_square.

@wbhart
Copy link
Contributor Author

wbhart commented Jul 30, 2020

@thofma @tthsqe12 What do you want me to do here? Are you happy for it to raise an exception (which can be caught) if the Flint function fq_sqrt returns 0? As it is, the Nemo function returns the square root if it is a square, so we can't return a boolean unless you want me to return a tuple.

Of course, changing the (proposed exception) behaviour here would imply changing it throughout Nemo.

@thofma
Copy link
Member

thofma commented Jul 30, 2020

I am happy with sqrt throwing if flint returns 0.

Regarding is issquare, I would be happy to introduce the tuple variant (and help rolling it out in the rest of Nemo). As of JuliaLang/julia#34126, tuples don't have the performance penalty that they used to have (they will be stack allocated).

@wbhart
Copy link
Contributor Author

wbhart commented Jul 30, 2020

That sounds a bit inconvenient. Instead of:

if issquare(a)
   # blah blah blah
end

it becomes:

if issquare(a)[1]
    # blah blah blah
end

Are you sure you want that? Moreover, an extra object is created to store the sqrt, even if you don't need it!

@thofma
Copy link
Member

thofma commented Jul 30, 2020

I don't mind the [1] and the additional object. It is IMHO better then doing the work twice if one has issquare(a) followed by an sqrt(a). Catching exceptions is just asking for trouble.

We have to do this unfortunately(?) quite often in Hecke. There are lots of function pairs like issquare/sqrt, e.g. isprincipal/principal_generator for ideals, where testing the attribute automatically computes the "proof" (I hope you know what I mean). So far we have not found a better way to solve this problem in julia.

@wbhart
Copy link
Contributor Author

wbhart commented Jul 30, 2020

Well, if everyone agrees I guess we can do it. I don't like the [1] very much, but I imagine I could live with it.

What is the current mechanism for figuring out what everyone agrees? It seems meetings are only for status reporting.

Should I just tag a bunch of people here?

@wbhart
Copy link
Contributor Author

wbhart commented Jul 30, 2020

If you want it, I suppose you could open a ticket first and see if there are any vocal complaints.

@tthsqe12
Copy link
Contributor

I dont like the issquare()[1]. Nor is catching an exception particularly attractive. If both the property and the object that proves the property true are desired in one function call, how about a third function, issquare_and_sqrt? :)

@thofma
Copy link
Member

thofma commented Jul 30, 2020

I will open an issue somewhere and we can discuss it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants