Opened 17 months ago
Closed 10 months ago
#31083 closed defect (fixed)
Fix for potential good reduction for dynamical systems
Reported by:  ghEnderWannabe  Owned by:  

Priority:  major  Milestone:  sage9.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: 
Description (last modified by )
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
 Branch set to u/ghEnderWannabe/good_reduction_fix
comment:2 Changed 17 months ago by
 Commit set to 8865571137d7e4f35e3bbf07a2bd93a5dd1b5a53
 Status changed from new to needs_review
comment:3 followup: ↓ 5 Changed 16 months ago by
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.
comment:4 Changed 16 months ago by
 Commit changed from 8865571137d7e4f35e3bbf07a2bd93a5dd1b5a53 to 6aff61faa957f9f6f4bb112451673cecdb99cfd5
Branch pushed to git repo; I updated commit sha1. New commits:
6aff61f  31083: fixed return type inconsistency, added return_conjugation, added tests

comment:5 in reply to: ↑ 3 Changed 16 months ago by
 Description modified (diff)
comment:6 Changed 16 months ago by
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
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
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 'dochtml' failed
comment:9 Changed 16 months ago by
 Commit changed from 6aff61faa957f9f6f4bb112451673cecdb99cfd5 to 26379c5a40015c27a0f59c74687faa5a77bd30ef
comment:10 Changed 16 months ago by
Docs should build now.
comment:11 Changed 16 months ago by
 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
 Commit changed from 26379c5a40015c27a0f59c74687faa5a77bd30ef to 330c7ae5f7784a997776ad6bf35f3b01e52f061a
Branch pushed to git repo; I updated commit sha1. New commits:
330c7ae  31083: added explicit example of correct conjugation and fixed embedding error

comment:13 Changed 15 months ago by
 Commit changed from 330c7ae5f7784a997776ad6bf35f3b01e52f061a to 67540210248922a9129a9757a7f9ffce5cf4ecf2
Branch pushed to git repo; I updated commit sha1. New commits:
6754021  31083: added second test of correct conjugation

comment:14 Changed 15 months ago by
 Commit changed from 67540210248922a9129a9757a7f9ffce5cf4ecf2 to 4df15d390fbedd8512db739b3f494e0f1cef61e0
Branch pushed to git repo; I updated commit sha1. New commits:
4df15d3  31083: added failing example as test

comment:15 Changed 15 months ago by
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 followup: ↓ 18 Changed 15 months ago by
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 (70757076)
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
 Commit changed from 4df15d390fbedd8512db739b3f494e0f1cef61e0 to 42fc94d790b6c306f9b52a3e4a5cf21dc50b7588
Branch pushed to git repo; I updated commit sha1. New commits:
42fc94d  31083: better fix for embedding

comment:18 in reply to: ↑ 16 Changed 15 months ago by
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
 Status changed from needs_work to needs_review
comment:20 Changed 15 months ago by
 Branch changed from u/ghEnderWannabe/good_reduction_fix to u/bhutz/good_reduction_fix
comment:21 Changed 15 months ago by
 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:
ea29310  31083: add doc test for conjugation

comment:22 Changed 15 months ago by
Looks good to me! Marked as positive review.
comment:23 Changed 15 months ago by
 Status changed from needs_review to positive_review
comment:24 Changed 15 months ago by
reviewer name should be full real name, please
comment:25 Changed 14 months ago by
 Reviewers changed from bhutz to Ben Hutz
comment:26 Changed 14 months ago by
sorry. Thanks for updating it.
comment:27 Changed 14 months ago by
 Status changed from positive_review to needs_work
Merge conflict
comment:28 Changed 14 months ago by
 Branch changed from u/bhutz/good_reduction_fix to u/ghEnderWannabe/good_reduction_fix
comment:29 Changed 14 months ago by
 Commit changed from ea29310ef2775664262c712c5f2256c469b447f3 to 2d838a1bf517ae3e2f4241d048560f87d2e49703
 Status changed from needs_work to needs_review
New commits:
2d838a1  31083: merge conflict resolved

comment:30 Changed 14 months ago by
 Branch changed from u/ghEnderWannabe/good_reduction_fix to u/bhutz/good_reduction_fix
comment:31 Changed 14 months ago by
 Commit changed from 2d838a1bf517ae3e2f4241d048560f87d2e49703 to ea29310ef2775664262c712c5f2256c469b447f3
err...that didn't fix the merge conflict. Try this one.
comment:32 Changed 14 months ago by
 Status changed from needs_review to needs_work
red branch => needs work
comment:33 Changed 14 months ago by
 Commit changed from ea29310ef2775664262c712c5f2256c469b447f3 to abc6978d1ba197ecba2a5f2129d6950fe96cf0a2
Branch pushed to git repo; I updated commit sha1. New commits:
abc6978  Merge branch 'master' into t/31083/good_reduction_fix

comment:34 Changed 14 months ago by
 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
ping
comment:36 Changed 14 months ago by
 Status changed from needs_review to positive_review
Looks good. Hopefully that resolves this merge conflict
comment:37 Changed 13 months ago by
 Milestone changed from sage9.3 to sage9.4
milestone to 9.4, as 9.3 has been released
comment:38 Changed 10 months ago by
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 keywordonly argument.
comment:39 Changed 10 months ago by
 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
 Branch changed from u/bhutz/good_reduction_fix to abc6978d1ba197ecba2a5f2129d6950fe96cf0a2
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
31083: initial commit
31083: full fix, added test
31083: final commit