-
-
Notifications
You must be signed in to change notification settings - Fork 710
Description
The function signature and default values for parameters for square roots of finite rings/fields is inconsistent. @user202729 pointed out this is a violation of the Liskov substitution principle. This was discussed in #40401 and determined to be out of scope for that PR.
A few examples of the inconsistencies:
- https://doc.sagemath.org/html/en/reference/finite_rings/sage/rings/finite_rings/integer_mod.html#sage.rings.finite_rings.integer_mod.IntegerMod_int.sqrt
sqrt(extend=True, all=False) - https://doc.sagemath.org/html/en/reference/finite_rings/sage/rings/finite_rings/element_base.html#sage.rings.finite_rings.element_base.FinitePolyExtElement.sqrt
sqrt(extend=True, all=False) - https://doc-develop--sagemath.netlify.app/html/en/reference/categories/sage/categories/finite_fields#sage.categories.finite_fields.FiniteFields.ElementMethods.sqrt
sqrt(all=False, algorithm='tonelli') - https://doc.sagemath.org/html/en/reference/finite_rings/sage/rings/finite_rings/integer_mod.html#sage.rings.finite_rings.integer_mod.IntegerMod_abstract.sqrt
sqrt(extend=True, all=False, name=None)
What values make sense for the algorithm parameter will vary between implementations. For that parameter specifically I think the various implementations should fallback to their default square root algorithm if the user passes an algorithm option that is not supported there. For example, algorithm=tonelli is supported for the finite field category implementation but doesn't make sense for the Givaro implementation because the Givaro square root algorithm is so efficient that there is no point in using anything else. So I suggest that Givaro should have an optional algorithm parameter to maintain consistency (Liskov substitution principle) but ignore it.
Since the algorithm parameter changes how something is computed but not what is returned, we can have different supported algorithms for the various implementations without worrying about violating the Liskov substitution principle. If we fallback to a default algorithm if the user asks to use an unknown algorithm, we could (not saying we should without a good reason) even remove algorithms without going through a deprecation period, as the code will still behave the same.
Unless there is a good reason to do otherwise, I think the default behaviour of extend and all should be consistent for all finite rings. This may require a some kind of deprecation period.