#30409 closed enhancement (fixed)

Optional ideal parameter for normalize_coordinates

Reported by: gh-EnderWannabe Owned by:
Priority: minor Milestone: sage-9.2
Component: dynamics Keywords: gsoc20
Cc: bhutz, paulfili Merged in:
Authors: Alexander Galarraga Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: fdc5569 (Commits, GitHub, GitLab) Commit: fdc5569630dc6775b1758343195d8da3ec1972f0
Dependencies: Stopgaps:

Status badges

Description

In niche use cases, it is helpful to normalize a dynamical system defined over a number field with respect to a p-adic absolute value. By adding an optional ideal parameter, we allow for normalization with respect to a p-adic absolute value. In this context, normalization is scaling so that one coefficient has absolute value 1 while the rest have absolute value less than 1.

Change History (20)

comment:1 Changed 16 months ago by gh-EnderWannabe

  • Branch set to u/gh-EnderWannabe/normalize_coordinates_enhancement

comment:2 Changed 16 months ago by gh-EnderWannabe

  • Commit set to 6dc7c5a24b847873e78305cc81145bf08cc700ae
  • Status changed from new to needs_review

New commits:

6dc7c5a30409: initial implementation

comment:3 Changed 16 months ago by bhutz

  • Status changed from needs_review to needs_work

merge conflict

comment:4 Changed 16 months ago by git

  • Commit changed from 6dc7c5a24b847873e78305cc81145bf08cc700ae to 6fd203e4d9b20cd239e64f5073231f6e2e86d6b5

Branch pushed to git repo; I updated commit sha1. New commits:

6fd203eMerge branch 'develop' into normalize_coordinates_enhancement

comment:5 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:6 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_review to needs_work

comment:7 Changed 16 months ago by bhutz

This initial implementation looks ok, but I'd like to see the ability to pass in a valuation as well, instead of only an ideal.

comment:8 Changed 16 months ago by git

  • Commit changed from 6fd203e4d9b20cd239e64f5073231f6e2e86d6b5 to 7d485e88078cf1c85011ac41840dd94f4f8eb685

Branch pushed to git repo; I updated commit sha1. New commits:

7d485e830409: added support for valuations

comment:9 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:10 Changed 16 months ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to needs_work

Only one complaint here. I think it would be better to use kwds and allow ideal= or valuation= instead of overloading the ideal parameter.

Otherwise, the functionality works as expected.

I was able to produce some cases where the phrasing of the error messages was not quite aligned with the issue, but these are not cases where the function should be working.

P.<x,y>=ProjectiveSpace(QQbar,1)
f=DynamicalSystem([x^2 + 2*x*y + 3*y^2, 4*y^2 + 5*x*y+7*x^2])
f.normalize_coordinates(ideal=QQbar(3))
ValueError: ideal was defined over QQ, but the base ring of this morphism is Algebraic Field

This is not an ideal defined over QQ, rather it is the ideal (1) in QQbar.

Similarly, this is an ideal contrary to what the error says

P.<x,y>=ProjectiveSpace(QQbar,1)
f=DynamicalSystem([x^2 + 2*x*y + 3*y^2, 4*y^2 + 5*x*y+7*x^2])
f.normalize_coordinates(ideal=QQbar.ideal(3))
TypeError: ideal must be an ideal or a valuation, not Principal ideal (1) of Algebraic Field

I'm not suggesting you special case things like QQbar in your error messages. Rather perhaps make the language somewhat more general. In this second you could say, must be a prime ideal or valuation. The first error may not a bigger language change.

comment:11 Changed 16 months ago by git

  • Commit changed from 7d485e88078cf1c85011ac41840dd94f4f8eb685 to a9105b3bd9f6a6fdc88b5e791b07cf0547b27119

Branch pushed to git repo; I updated commit sha1. New commits:

a9105b330409: use kwargs, better error messages

comment:12 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:13 Changed 16 months ago by bhutz

  • Status changed from needs_review to needs_work

Those errors I think are less confusing.

two things

  • kwds is what should be used
  • the kwds should be in a keywords: block of the INPUT section,

comment:14 Changed 16 months ago by git

  • Commit changed from a9105b3bd9f6a6fdc88b5e791b07cf0547b27119 to 833e87473db60f88d019d9bb216646d6c997b75f

Branch pushed to git repo; I updated commit sha1. New commits:

833e87430409: fixed keywords

comment:15 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:16 Changed 16 months ago by bhutz

  • Status changed from needs_review to needs_work

You need to change the kwargs in the code to match

ideal = kwargs.pop('ideal', None)

and the other one.

comment:17 Changed 16 months ago by git

  • Commit changed from 833e87473db60f88d019d9bb216646d6c997b75f to fdc5569630dc6775b1758343195d8da3ec1972f0

Branch pushed to git repo; I updated commit sha1. New commits:

fdc556930409: fixed kwds

comment:18 Changed 16 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:19 Changed 15 months ago by bhutz

  • Status changed from needs_review to positive_review

comment:20 Changed 15 months ago by vbraun

  • Branch changed from u/gh-EnderWannabe/normalize_coordinates_enhancement to fdc5569630dc6775b1758343195d8da3ec1972f0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.