#31083 closed defect (fixed)

Fix for potential good reduction for dynamical systems

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

Status badges

Description (last modified by gh-EnderWannabe)

The function potential_good_reduction states that it returns a dynamical system with good reduction at a specified prime.

Currently, the following example returns a dynamical system:

P.<x,y> = ProjectiveSpace(QQ, 1)
system = DynamicalSystem_projective([3^5*x^3 + x^2*y - 3^5*x*y^2 , -3^5*x^2*y + x*y^2 + 3^5*y^3])
system.potential_good_reduction(3)

However, it is proved in https://arxiv.org/pdf/1304.1201.pdf that this dynamical system does not have potential good reduction at 3.

Following the criterion in https://arxiv.org/pdf/1311.6695.pdf, a fix is implemented by first checking the valuation of the resultant of the conjugate system before returning.

In order to be consistent, we change the function return type so that a tuple is returned in all cases.

Furthermore, we add an optional return conjugation parameter.

Change History (40)

comment:1 Changed 17 months ago by gh-EnderWannabe

  • Branch set to u/gh-EnderWannabe/good_reduction_fix

comment:2 Changed 17 months ago by gh-EnderWannabe

  • Commit set to 8865571137d7e4f35e3bbf07a2bd93a5dd1b5a53
  • Status changed from new to needs_review

New commits:

668edfc31083: initial commit
0a0775731083: full fix, added test
886557131083: final commit

comment:3 follow-up: Changed 16 months ago by bhutz

I took a look at this (finally) and it looks fine to me. If I'm understanding the references correctly, the conjugation you are producing must be the one that gives good reduction, so you check if the resulting map does, otherwise it does not.

Maybe this is not the place to discuss this, but I'm a little concerned about the fact that this function returns different types depending on the result. I'd be a little inclined to change the returns to

True, dynamicalsystem

False, None

Although, why is there no option to return the conjugation? So perhaps as a second option

True, DS, PGL

False, None, None

That way the returns are always consistent.

Since this is not yet in a released version, we're still free to change such things without having to worry about deprecation.

Last edited 16 months ago by bhutz (previous) (diff)

comment:4 Changed 16 months ago by git

  • Commit changed from 8865571137d7e4f35e3bbf07a2bd93a5dd1b5a53 to 6aff61faa957f9f6f4bb112451673cecdb99cfd5

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

6aff61f31083: fixed return type inconsistency, added return_conjugation, added tests

comment:5 in reply to: ↑ 3 Changed 16 months ago by gh-EnderWannabe

  • Description modified (diff)

comment:6 Changed 16 months ago by gh-EnderWannabe

Replying to bhutz:

Maybe this is not the place to discuss this, but I'm a little concerned about the fact that this function returns different types depending on the result.

Implemented the change suggested, and updated the ticket description. Should we consider separating these changes into two separate tickets?

comment:7 Changed 16 months ago by bhutz

These are both small fixes to the same function under beta, so I'm ok doing them both here.

Paul it looks like you reviewed the original function. Are you ok with the new return types?

comment:8 Changed 16 months ago by bhutz

fyi, the docs fail to build

/arithmetic_dynamics/projective_ds.py:docstring of sage.dynamics.arithmetic_dynamics.projective_ds.DynamicalSystem_projective_field.potential_good_reduction:37: WARNING: Unexpected indentation.
Makefile:1873: recipe for target 'doc-html' failed

comment:9 Changed 16 months ago by git

  • Commit changed from 6aff61faa957f9f6f4bb112451673cecdb99cfd5 to 26379c5a40015c27a0f59c74687faa5a77bd30ef

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

d4a212a31803: fixed undefined variable L
26379c531083: doc fix

comment:10 Changed 16 months ago by gh-EnderWannabe

Docs should build now.

comment:11 Changed 16 months ago by bhutz

  • Reviewers set to bhutz
  • Status changed from needs_review to needs_work

This looks good to me. Just one further tweak I'd like. A test that checks that the conjugation returned actually gives the model with good reduction.

comment:12 Changed 15 months ago by git

  • Commit changed from 26379c5a40015c27a0f59c74687faa5a77bd30ef to 330c7ae5f7784a997776ad6bf35f3b01e52f061a

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

330c7ae31083: added explicit example of correct conjugation and fixed embedding error

comment:13 Changed 15 months ago by git

  • Commit changed from 330c7ae5f7784a997776ad6bf35f3b01e52f061a to 67540210248922a9129a9757a7f9ffce5cf4ecf2

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

675402131083: added second test of correct conjugation

comment:14 Changed 15 months ago by git

  • Commit changed from 67540210248922a9129a9757a7f9ffce5cf4ecf2 to 4df15d390fbedd8512db739b3f494e0f1cef61e0

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

4df15d331083: added failing example as test

comment:15 Changed 15 months ago by gh-EnderWannabe

Added the desired example.

I found an example which failed causing an embedding error:

P.<x,y> = ProjectiveSpace(QQ, 1)
system = DynamicalSystem_projective([3*x^2 + x*y+y^2, 9*y^2])
prime = system.field_of_definition_periodic(1).prime_above(3)
system.potential_good_reduction(prime)

The most recent pushes fixes this and add it as a test. Some of the code I wrote to fix it, however, I am unhappy with.

Since the change ring on projective points doesn't let me pass an embedding, I tried to use the embedding by hand on line 7076. Check that line to see if I missed something there.

Also, I resorted to checking if an embedding is defined on line 7094. Is that good practice? I couldn't come up with another approach, as using the identity when embedding_preimage was not defined failed tests.

comment:16 follow-up: Changed 15 months ago by bhutz

err..why can't you pass en embedding into the projective point change_ring. I'm under the impression this works and changing your code to (7075-7076)

            system = system.change_ring(embedding_preimage)
            point = point.change_ring(embedding_preimage)

does not cause any doctest failures. Could you give me an example that is failing under this?

btw, you have at least one doctest failure in general that needs to be fixed (7029: does not have the return value to check against.)

comment:17 Changed 15 months ago by git

  • Commit changed from 4df15d390fbedd8512db739b3f494e0f1cef61e0 to 42fc94d790b6c306f9b52a3e4a5cf21dc50b7588

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

42fc94d31083: better fix for embedding

comment:18 in reply to: ↑ 16 Changed 15 months ago by gh-EnderWannabe

Thanks for the suggestion, that works much better. I didn't realize I could just pass the embedding.

Failing doctest fixed! I'm going to change the status to needs review, as I think all the current issues have been addressed.

comment:19 Changed 15 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:20 Changed 15 months ago by bhutz

  • Branch changed from u/gh-EnderWannabe/good_reduction_fix to u/bhutz/good_reduction_fix

comment:21 Changed 15 months ago by bhutz

  • Commit changed from 42fc94d790b6c306f9b52a3e4a5cf21dc50b7588 to ea29310ef2775664262c712c5f2256c469b447f3

I actually meant checking that you are getting the actual map you return not just that it has the correct resultant. See the test I added.

Otherwise, I'm fine with this ticket as is. If you are also fine with the doctest I added, we can mark this positive.


New commits:

ea2931031083: add doc test for conjugation

comment:22 Changed 15 months ago by gh-EnderWannabe

Looks good to me! Marked as positive review.

comment:23 Changed 15 months ago by gh-EnderWannabe

  • Status changed from needs_review to positive_review

comment:24 Changed 15 months ago by chapoton

reviewer name should be full real name, please

comment:25 Changed 14 months ago by chapoton

  • Reviewers changed from bhutz to Ben Hutz

comment:26 Changed 14 months ago by bhutz

sorry. Thanks for updating it.

comment:27 Changed 14 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:28 Changed 14 months ago by gh-EnderWannabe

  • Branch changed from u/bhutz/good_reduction_fix to u/gh-EnderWannabe/good_reduction_fix

comment:29 Changed 14 months ago by gh-EnderWannabe

  • Commit changed from ea29310ef2775664262c712c5f2256c469b447f3 to 2d838a1bf517ae3e2f4241d048560f87d2e49703
  • Status changed from needs_work to needs_review

New commits:

2d838a131083: merge conflict resolved

comment:30 Changed 14 months ago by bhutz

  • Branch changed from u/gh-EnderWannabe/good_reduction_fix to u/bhutz/good_reduction_fix

comment:31 Changed 14 months ago by bhutz

  • Commit changed from 2d838a1bf517ae3e2f4241d048560f87d2e49703 to ea29310ef2775664262c712c5f2256c469b447f3

err...that didn't fix the merge conflict. Try this one.

comment:32 Changed 14 months ago by chapoton

  • Status changed from needs_review to needs_work

red branch => needs work

comment:33 Changed 14 months ago by git

  • Commit changed from ea29310ef2775664262c712c5f2256c469b447f3 to abc6978d1ba197ecba2a5f2129d6950fe96cf0a2

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

abc6978Merge branch 'master' into t/31083/good_reduction_fix

comment:34 Changed 14 months ago by bhutz

  • Status changed from needs_work to needs_review

Apparently I failed to commit the merge conflict fix. I've done so now.

comment:35 Changed 14 months ago by bhutz

ping

comment:36 Changed 14 months ago by gh-EnderWannabe

  • Status changed from needs_review to positive_review

Looks good. Hopefully that resolves this merge conflict

comment:37 Changed 13 months ago by chapoton

  • Milestone changed from sage-9.3 to sage-9.4

milestone to 9.4, as 9.3 has been released

comment:38 Changed 10 months ago by mkoeppe

             return gccc, m * mc * mc2, psi
         return gccc
 
-    def potential_good_reduction(self, prime):
+    def potential_good_reduction(self, prime, return_conjugation=False):
         r"""
-        Return if this dynamical system has potential good reduction at ``prime``.
+        Return ``True`` if this dynamical system has potential good reduction at ``prime``.
 
         A dynamical system has good reduction at ``prime`` if after the coefficients

In light of the discussion in #32205, this could proactively be changed to def potential_good_reduction(self, prime, *, return_conjugation=False), to declare return_conjugation to be a keyword-only argument.

comment:39 Changed 10 months ago by mkoeppe

  • Priority changed from minor to major

Promoting 5 tickets that fix defects to "major" so that they have a chance to get merged

comment:40 Changed 10 months ago by vbraun

  • Branch changed from u/bhutz/good_reduction_fix to abc6978d1ba197ecba2a5f2129d6950fe96cf0a2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.