Skip to content

Conversation

@Newtech66
Copy link
Contributor

@Newtech66 Newtech66 commented May 4, 2023

📚 Description

Changed all instances of impl to implementation in sage.rings.finite_rings. Also added deprecation warning for impl in finite_field_constructor.py.

Fixes #30507.

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@roed314
Copy link
Contributor

roed314 commented May 4, 2023

Generally looks good; I've approved the test run and once tests pass I'm happy with this PR.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 4, 2023

Lots of doctests are failing:

      File "sage/categories/functor.pyx", line 382, in sage.categories.functor.Functor.__call__
        y = self._apply_functor(self._coerce_into_domain(x))
      File "/__w/sage/sage/src/sage/categories/pushout.py", line 505, in _apply_functor
        R = c(R)
      File "sage/categories/functor.pyx", line 382, in sage.categories.functor.Functor.__call__
        y = self._apply_functor(self._coerce_into_domain(x))
      File "/__w/sage/sage/src/sage/categories/pushout.py", line 3290, in _apply_functor
        return R.extension(self.polys[0], names=self.names[0], embedding=self.embeddings[0],
    KeyError: 'implementation'

@Newtech66
Copy link
Contributor Author

Newtech66 commented May 5, 2023

There seems to be an issue with this bit of code:

sage: K = GF(2^3,'a')
sage: F,O = K.construction()
sage: F(O)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
Cell In [43], line 1
----> 1 F(O)

File ~/sage/src/sage/categories/functor.pyx:382, in sage.categories.functor.Functor.__call__()
    380 if is_Morphism(x):
    381     return self._apply_functor_to_morphism(x)
--> 382 y = self._apply_functor(self._coerce_into_domain(x))
    383 if not ((y in self.__codomain) or (y in self.__codomain.Homsets())):
    384     raise TypeError("%s is ill-defined, since it sends x (=%s) to something that is not in %s." % (repr(self), x, self.__codomain))

File ~/sage/src/sage/categories/pushout.py:3290, in AlgebraicExtensionFunctor._apply_functor(self, R)
   3288     return R.residue_field(R*self.residue, names=tuple(self.names))
   3289 if len(self.polys) == 1:
-> 3290     return R.extension(self.polys[0], names=self.names[0], embedding=self.embeddings[0],
   3291                        structure=self.structures[0], prec=self.precs[0],
   3292                        implementation=self.implementations[0],
   3293                        latex_names=self.latex_names[0], **self.kwds)
   3294 return R.extension(self.polys, names=self.names, embedding=self.embeddings,
   3295                    structure=self.structures, prec=self.precs,
   3296                    implementation=self.implementations,
   3297                    latex_names=self.latex_names, **self.kwds)

TypeError: FiniteField_prime_modn_with_category.extension() got multiple values for keyword argument 'implementation'

The problem is that when F,O = K.construction() is called, F (an instance of AlgebraicExtensionFunctor) gets a keyword implementation: givaro (lines 1419-1458 in sage/rings/finite_rings/finite_field_base.pyx) from K (which is of type class <sage.rings.finite_rings.finite_field_givaro.FiniteField_givaro_with_category>). Then when F(O) is called, the function _apply_functor passes implementation again along with the previous one and now there is a clash.

How to resolve this? It doesn't look like F needs this keyword, so perhaps it should be deleted in __init__? I am referencing line 3475 in sage/categories/pushout.py for this suggestion.

@Newtech66
Copy link
Contributor Author

But the implementation needs to be taken into account. Adding the passed implementation keyword to self.implementations seems to fix the problem.

@Newtech66
Copy link
Contributor Author

All previously failing doctests are now passing on my system.

@Newtech66
Copy link
Contributor Author

Can it be reviewed?

@Newtech66 Newtech66 requested a review from kwankyu July 13, 2023 07:54
@kwankyu
Copy link
Collaborator

kwankyu commented Jul 21, 2023

Workflows are awaiting approval from a maintainer.

@github-actions
Copy link

Documentation preview for this PR (built with commit 95e3bc6; changes) is ready! 🎉

if 'impl' in kwds_self:
del kwds_self['impl']
if 'implementation' in kwds_self:
del kwds_self['implementation']
Copy link
Collaborator

Choose a reason for hiding this comment

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

L3553,3554 could also be deleted, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sage.rings.finite_rings: Change impl to implementation

5 participants