-
Notifications
You must be signed in to change notification settings - Fork 166
Optimize _find_elem_type helper
#5574
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
base: master
Are you sure you want to change the base?
Conversation
3391baa to
db0f1ea
Compare
|
|
||
| _parent_or_coefficient_field(r::Base.RefValue{<:Union{FieldElem,ZZRingElem}}, x...) = | ||
| parent(r.x) | ||
| parent(r[]) |
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 change is unrelated; it replaces access to undocumented internals of Base.RefValue by the documented way to access the content of the ref.
db0f1ea to
eb150b4
Compare
The new version avoids allocations in many 'obvious' case.
For example, before:
julia> a = [0 0; 1 0; 1 1; 0 1];
julia> @b Oscar._guess_fieldelem_type($a)
1.265 μs (4 allocs: 256 bytes)
julia> @b Oscar._guess_fieldelem_type($a, $a)
5.750 μs (25 allocs: 1.156 KiB)
julia> @b Oscar._guess_fieldelem_type($a, $a, $a)
8.500 μs (37 allocs: 1.844 KiB)
After:
julia> a = [0 0; 1 0; 1 1; 0 1];
julia> @b Oscar._guess_fieldelem_type($a)
2.161 ns
julia> @b Oscar._guess_fieldelem_type($a, $a)
2.468 ns
julia> @b Oscar._guess_fieldelem_type($a, $a, $a)
2.781 ns
eb150b4 to
fa72ab0
Compare
| t = _find_elem_type(y) | ||
| t === Any && continue | ||
| t === Float64 && return Float64 | ||
| T = promote_type(t, T) |
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.
I don't think it will work with using the result of promote_type, in many cases this will just give FieldElem, e.g.:
julia> promote_type(QQFieldElem,QQBarFieldElem)
FieldElem
but if that is different from QQFieldElem we can assume that we should use the element that we are just checking, i.e., something like promote_type(t, T) != T && T = t.
Edit: I am somewhat surprised that it does work now.
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.
It looks like we need more tests for this, but it does not work as expected:
julia> mm = QQBarFieldElem[1 2 3; 1 3 qb; 4 5 6]
3×3 Matrix{QQBarFieldElem}:
{a1: 1.00} {a1: 2.00} {a1: 3.00}
{a1: 1.00} {a1: 3.00} {a2: 1.30}
{a1: 4.00} {a1: 5.00} {a1: 6.00}
julia> Oscar._guess_fieldelem_type(mm)
FieldElem
(on master this returns QQBarFieldElem)
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.
A description of what the goal resp. algorithm is, roughly, would go a long way. As in: find a concrete (?) field (?) elem type to which all elements in the given tuples/arrays/MatElem (and recursive constructions thereof) may be converted "
and I assume the actual allowed output types are actually a small fixed set? But which is it?
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.
In practice (at least for now) we use QQFieldElem, Float64, QQBarFieldElem, and <:EmbeddedNumFieldElem.
Theoretically any ordered FieldElem (with an embedding of the rationals) can be stored in a Polymake.OscarNumber and passed to polymake.
The code tries to be generic, some parts were written before the support for QQBar but then went through some major refactor. But there is no fixed list of result types.
The goal is to get one reasonable field type that works for all input arguments, which can a mix of julia or Oscar matrices, vectors, single elements, and SubObjectIterators from existing polyhedral objects.
This type will then be passed to _parent_or_coefficient_field to get a proper parent object.
All this should preferably only be used for user input when constructing polymake BigObjects (which will cause plenty of allocations anyway, most of which will not be even be visible to any benchmark commands).
Any Oscar code calling these constructions should pass the field and avoid this auto-detection.
The new version avoids allocations in many 'obvious' case.
For example, before:
After:
(I have no idea why the 2 argument version is so much slower but also did not try to look since it is "good enough" for me that it doesn't allocate anymore.UPDATE: nevermind, the session in which I conducted the benchmark was tainted, in a fresh session the numbers are sane (and better)