Fix AttributeError in PowerSeriesRing for division

[vidipsingh]

This PR resolves a bug where attempting to divide a power series element, like x/(1-z), in PowerSeriesRing_over_field_with_category would result in an AttributeError due to the missing _pseudo_fraction_field attribute. The fix ensures that operations involving division within power series rings work as expected, similar to multiplication.

Fixes: #39684

📝 Checklist

• The title is concise and informative.
• 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 and checked the documentation preview.

⌛ Dependencies

[nbruin]

Note that the failure is recent. It used to work just fine. One should first find what change broke the example and then investigate how that breakage can be mitigated. Certainly, any division operation should get a chance to run through the coercion framework, so implementing a fresh __truediv__ is not the right solution. You'd need to hook into the coercion framework with other routines.

It may well be possible that power series rings were previously not properly cooperating in the coercion framework and that the breakage is due to an attempt to fix this, which ended up having some flaws that need further attention.

In older versions we find:

sage: coercion_model.explain(y,(1-x),operator.truediv)
Action discovered.
    Right inverse action by Laurent Series Ring in x over Rational Field on Power Series Ring in y over Power Series Ring in x over Rational Field
    with precomposition on right by Coercion map:
      From: Power Series Ring in x over Rational Field
      To:   Laurent Series Ring in x over Rational Field
Result lives in Power Series Ring in y over Laurent Series Ring in x over Rational Field
Power Series Ring in y over Laurent Series Ring in x over Rational Field

I didn't check if that is also what is actually done, but it shows that the coercion framework is perfectly capable of figuring out what operation to do.

[vidipsingh]

Note that the failure is recent. It used to work just fine. One should first find what change broke the example and then investigate how that breakage can be mitigated. Certainly, any division operation should get a chance to run through the coercion framework, so implementing a fresh __truediv__ is not the right solution. You'd need to hook into the coercion framework with other routines.

It may well be possible that power series rings were previously not properly cooperating in the coercion framework and that the breakage is due to an attempt to fix this, which ended up having some flaws that need further attention.

In older versions we find:

sage: coercion_model.explain(y,(1-x),operator.truediv)
Action discovered.
    Right inverse action by Laurent Series Ring in x over Rational Field on Power Series Ring in y over Power Series Ring in x over Rational Field
    with precomposition on right by Coercion map:
      From: Power Series Ring in x over Rational Field
      To:   Laurent Series Ring in x over Rational Field
Result lives in Power Series Ring in y over Laurent Series Ring in x over Rational Field
Power Series Ring in y over Laurent Series Ring in x over Rational Field

I didn't check if that is also what is actually done, but it shows that the coercion framework is perfectly capable of figuring out what operation to do.

Thanks for the feedback! I realize __truediv__ skips the coercion framework, which isn’t ideal. I’ll track down the recent change breaking PowerSeriesRing division and fix it there.
Could you point me to where its coercion setup is defined? I’ll update the PR after finding the regression.

[nbruin]

Possibly useful piece of info, in 10.5.beta6 we still had

sage: R.<x>=QQ[[]]
sage: type(R)._pseudo_fraction_field
<method '_pseudo_fraction_field' of 'sage.rings.ring.CommutativeRing' objects>

so it used to work by just inheriting from the normal place. The first place to look for breakage is into why that inheritance doesn't happen anymore.

In fact, with git-blame the problem is quickly identified:
1480abb merged with pull request #38741
removes the inheritance from sage.rings.ring.CommutativeRing. Pinging author @fchapoton for info on why that was desirable and justified to do. It would look like a power series ring would be able to get some useful functionality from CommutativeRing; even if its implementation is only an approximation of one.

[vidipsingh]

Possibly useful piece of info, in 10.5.beta6 we still had

sage: R.<x>=QQ[[]]
sage: type(R)._pseudo_fraction_field
<method '_pseudo_fraction_field' of 'sage.rings.ring.CommutativeRing' objects>

so it used to work by just inheriting from the normal place. The first place to look for breakage is into why that inheritance doesn't happen anymore.

In fact, with git-blame the problem is quickly identified: 1480abb merged with pull request #38741 removes the inheritance from sage.rings.ring.CommutativeRing. Pinging author @fchapoton for info on why that was desirable and justified to do. It would look like a power series ring would be able to get some useful functionality from CommutativeRing; even if its implementation is only an approximation of one.

Hi @nbruin, thanks for the pointer! Here’s what I found and did:

• 10.5.beta6: PowerSeriesRing_generic got _pseudo_fraction_field from CommutativeRing, enabling division (e.g., x / (1 - x)) via LaurentSeriesRing.
• Breakage: Commit 1480abb (move power series to Parent #38741) by @fchapoton switched to Parent, dropping _pseudo_fraction_field and causing Unexpected AttributeError: 'PowerSeriesRing_over_field_with_category' object has no attribute '_pseudo_fraction_field' #39684’s AttributeError.
• Fix: Added to PowerSeriesRing_generic:

  def _pseudo_fraction_field(self):
      return LaurentSeriesRing(self.base_ring(), self._names[0])

Is this the right approach? Could you provide feedback on it?

[nbruin]

A few points:

• this bug here shows that changing the inheritance from CommutativeRing to Parent has consequences that weren't considered when the change was made. @fchapoton will be able to comment. Probably a more comprehensive review of the change (whether it is advisable at all and if so how it should be accommodated) is warranted.
• Previously _pseudo_fraction_field was inherited from elsewhere. That may still be a good idea (if PowerSeriesRing isn't inheriting from CommutativeRing anymore there are probably other rings that shouldn't either and they will have similar problems)
• Previously _pseudo_fraction_field was implemented by calling fraction_field. That should probably still happen. Or, if implementing _pseudo_fraction_field becomes the responsibility of the concrete class then one could simply alias the fraction_field method to _pseudo_fraction_field. Note that presently, fraction_field gets implemented on subclasses below. So _pseudo_fraction_field should definitely defer to those more specialized methods.

Main takeaway: this bug is indicative of a more systemic issue and it's probably better to deal with it on a higher level. Otherwise, the responsibilities for where _pseudo_fraction_field is to be implemented should be documented.

[fchapoton]

Yes, this method "_pseudo_fraction_field", whatever it does, should have a default implementation in the category of commutative rings. This is an oversight of my changes. The ultimate goal is to get rid of all the historical classes like Ring and CommutativeRing. We are in the process of moving methods one by one, as carefully as possible.

[nbruin]

@fchapoton OK, so it sounds this ticket should be repurposed to: make sure all functionality of sage.ring.ring.CommutativeRing is faithfully replicated on .... Can you help the author on this ticket with what should go on the dots and, more importantly, where that can be found in the source tree? With explicit inheritance, general python skills tell you where to look. As soon as the category framework gets involved it's not clear to most programmers where to look (and indeed I don't know either).

[nbruin]

@vidipsingh You should probably read https://doc.sagemath.org/html/en/reference/categories/sage/categories/primer.html since that's definitely going to be relevant. I suspect that the previous implementation:

    def _pseudo_fraction_field(self):
        """doc and tests go here"""
        try:
            return self.fraction_field()
        except (NotImplementedError,TypeError):
            return coercion_model.division_parent(self)

should probably go in parent_methods on sage.categories.rings.Rings.Commutative. It may be worth checking what other methods from sage.rings.ring.CommutativeRing should be copied there. There's a good chance a bunch of things already get inherited from elsewhere, so care would need to be taken for each routine separately. @fchapoton will hopefully be able to give some guidance, because of prior experience with transporting old-style parents to the category framework.

[fchapoton]

It is not reasonable to move several methods at once. Already just moving one method leads to bad surprises and more work than expected. This ticket should just be for moving "_pseudo_fraction_field".

[nbruin]

OK, that's useful. I guess an interesting experiment could be to see how much breaks when _pseudo_fraction_field is deleted from CommutativeRing after being implemented on Ring.Commutative.

Looking at where _pseudo_fraction_field is defined, I suspect we should also move one to fields.Fields. The only place where this method is overridden to not just be fraction_field (or just "self" on fields) is on "integers mod n". Makes sense: in rings where all non-zero-divisors are units, the parent for inverted elements can just be the ring itself.

Note that the breakage observed here was not detected by doctests, so this is probably machinery that has very poor test coverage.

[vidipsingh]

Thank you @nbruin and @fchapoton for your valuable feedback.

• Commit move power series to Parent #38741 removed CommutativeRing inheritance, eliminating _pseudo_fraction_field.
• This PR adds _pseudo_fraction_field to PowerSeriesRing_generic, returning LaurentSeriesRing, resolving Unexpected AttributeError: 'PowerSeriesRing_over_field_with_category' object has no attribute '_pseudo_fraction_field' #39684.
• It’s a focused fix, not fully integrated with coercion, adhering to single-method scope.
• I plan to merge this with doctests, then propose a Rings.Commutative solution in a new ticket.

Is this approach acceptable for now, or should it incorporate fraction_field?
What guidance can you offer for the category implementation?

[nbruin]

With the investigations done on this ticket I think it's become clear that _pseudo_fraction_field needs to be implemented in the parent_methods of the category Rings.Commutative to complete the transition of initiated in #38741 . The branch currently attached to this pull request should be abandoned in favour of that approach. It's not more work to fix this properly.

You mention you plan to propose at Rings.Commutative solution eventually. If you really want, you can of course keep working on the solution you started with here, but that won't get merged. The other solution likely will. So I'd suggest you start working on that. Whether you do that on a new PR or whether you force push a branch here is up to you.

The change should probably look something like this:

develop...nbruin:sage:_pseudo_fraction_field-on-Rings.Commutative

[nbruin]

@vidipsingh are you still planning to proceed with trying to address the issue on this PR? Or should we resolve the issue via a PR based on develop...nbruin:sage:_pseudo_fraction_field-on-Rings.Commutative ?

[vidipsingh]

@vidipsingh are you still planning to proceed with trying to address the issue on this PR? Or should we resolve the issue via a PR based on develop...nbruin:sage:_pseudo_fraction_field-on-Rings.Commutative ?

Hi @nbruin, yes I still want to proceed with the issue in this PR. My exams are going on, which is why I haven't been able to work on it, but I will start working as soon as I get the time.

[vidipsingh]

With the investigations done on this ticket I think it's become clear that _pseudo_fraction_field needs to be implemented in the parent_methods of the category Rings.Commutative to complete the transition of initiated in #38741 . The branch currently attached to this pull request should be abandoned in favour of that approach. It's not more work to fix this properly.

You mention you plan to propose at Rings.Commutative solution eventually. If you really want, you can of course keep working on the solution you started with here, but that won't get merged. The other solution likely will. So I'd suggest you start working on that. Whether you do that on a new PR or whether you force push a branch here is up to you.

The change should probably look something like this:

develop...nbruin:sage:_pseudo_fraction_field-on-Rings.Commutative

Hi @nbruin,

Thank you for the suggestion. I will proceed with implementing _pseudo_fraction_field in the parent_methods of the Rings.Commutative category as you've recommended. I'll force-push a new branch to this PR with those changes shortly. Any guidance or feedback you can offer would be greatly appreciated!

[nbruin]

@vidipsingh That patch looks fine to me. It's also literally the same patch as what I committed on my branch; down to the added doctest. So I think this is a good opportunity for you to learn about using other people's work with appropriate attribution.

The "author" information for commits like this are carried in the git history, with the possibility of adding authorship information to the source file for significant and substantial contributions (I don't think this passes that bar). So I think the appropriate measure on your side is to include in the commit message where you sourced this code. A github URL to a branch in someone's private fork is not going to be very stable, so in this case I think just adding "code contributed by Nils Bruin" would sufficient. I don't really care about the credit for this very trivial piece of code, but I think it is important for new contributors to pay attention to attribution.

It may well be that we find out after testing this commit that there are some further changes and/or doctests necessary. Those would then be fully your commits.

An alternative would be for you to fetch my branch into your own sagemath git and then force-push that one the pull-request here. Then you'd start out with a commit that has the original author info and you could then put further commits on that as necessary.

[vidipsingh]

@nbruin I have included your name in the commit message description as you suggested. Apologies for not adding authorship initially—I'll be more mindful of proper attribution in the future.
Let me know if this looks good or if any further changes are needed.

Thanks for pointing this out!

[github-actions]

Documentation preview for this PR (built with commit 740aaea; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[nbruin]

CI tests are not working. Perhaps that's because the branch is out-of-date. We do need to see that the tests complete successfully on this branch before approving it for merge.

Actually, the logs of the tests show that something indeed went wrong:

https://github.com/sagemath/sage/actions/runs/14153599086/job/39650014464?pr=39713#step:10:2264