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

Valuation on power series and Laurent series #39517

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

xcaruso
Copy link
Contributor

@xcaruso xcaruso commented Feb 13, 2025

We implement the method valuation on the rings of power series and Laurent series.

📝 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.

@xcaruso xcaruso added the sd128 tickets of Sage Days 128 Le Teich label Feb 13, 2025
@xcaruso xcaruso requested a review from saraedum February 13, 2025 17:33
@xcaruso
Copy link
Contributor Author

xcaruso commented Feb 13, 2025

I still have a problem with categories that I do not understand. Please tell me if you have some insight.


from sage.rings.valuation.valuation import DiscreteValuation

class SeriesValuation(DiscreteValuation):
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered making this a UniqueRepresentation? Then you can throw out _eq_ and __reduce__ from your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tried it (not so hard). But got an error that I did not really understand (something with metaclasses).

Copy link
Member

@saraedum saraedum Feb 13, 2025

Choose a reason for hiding this comment

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

Ah, that's annoying. Then you need to add a silly little factory.

Let me know if you need any support here (I can add it if you want me to.)

@mantepse
Copy link
Contributor

could you add this simultaneously to the lazy power / Laurent series?

or should this be actually a method in a category?

Comment on lines 315 to 316
valuation_space = DiscretePseudoValuationSpace(self)
return SeriesValuation(valuation_space)
Copy link
Member

Choose a reason for hiding this comment

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

You want to do valuation_space.__make_element_class__(SeriesValuation)(valuation_space) to get the category right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

@xcaruso
Copy link
Contributor Author

xcaruso commented Feb 13, 2025

could you add this simultaneously to the lazy power / Laurent series?

Sure, I will do it.

sage: v # indirect doctest
t-adic valuation
"""
return "%s-adic valuation" % self.domain().uniformizer()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return "%s-adic valuation" % self.domain().uniformizer()
return "%s-adic valuation" % self.uniformizer()

I think it would be good to implement uniformizer() on the level of valuation.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, once the "category" test above passes, more tests coming from the category of discrete valuations are going to fail, namely that you are missing a bunch of methods: uniformizer, reduce, lift, residue_field.

Copy link

github-actions bot commented Feb 13, 2025

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

@xcaruso
Copy link
Contributor Author

xcaruso commented Feb 13, 2025

Thanks for your help. I think that everything is working correctly now.


def create_object(self, version, key, **extra_args):
r"""
Create a unique key identifying the valuation of ``R``.
Copy link
Member

Choose a reason for hiding this comment

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

Actually this creates the object. That's the docstring for the other method I guess.

SeriesValuation = SeriesValuationFactory("sage.rings.series_valuation.SeriesValuation")


class SeriesValuation_base(DiscreteValuation):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe _generic since there's nobody inheriting from this. Base sounds like abstract base to me (just a suggestion. Keep it if you want.)

@saraedum
Copy link
Member

doctests fail with:

File "src/sage/rings/series_valuation.py", line 30, in sage.rings.series_valuation.SeriesValuationFactory
Failed example:
    v = R.valuation()
Exception raised:
    Traceback (most recent call last):
      File "/home/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 728, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/doctest/forker.py", line 1152, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.series_valuation.SeriesValuationFactory[1]>", line 1, in <module>
        v = R.valuation()
            ^^^^^^^^^^^^^
      File "/home/runner/miniconda3/envs/sage-dev/lib/python3.11/site-packages/sage/rings/power_series_ring.py", line 672, in valuation
        from sage.rings.series_valuation import SeriesValuation
    ModuleNotFoundError: No module named 'sage.rings.series_valuation'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: needs review sd128 tickets of Sage Days 128 Le Teich
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants