Opened 4 years ago

Closed 4 years ago

#17323 closed enhancement (fixed)

Implement "primes_of_bad_reduction" to work over Number Fields

Reported by: jdefaria Owned by:
Priority: major Milestone: sage-6.6
Component: algebraic geometry Keywords:
Cc: Merged in:
Authors: Joao Alberto de Faria Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 8e2195f (Commits) Commit: 8e2195fcfd470d6327eddf87d3e8869aed18c450
Dependencies: Stopgaps:

Description

The current code uses grobner basis, as such, there is no reason to restrict to QQ and ZZ only

Change History (23)

comment:1 Changed 4 years ago by jdefaria

  • Branch set to u/jdefaria/ticket/17323
  • Created changed from 11/12/14 17:10:52 to 11/12/14 17:10:52
  • Modified changed from 11/12/14 17:10:52 to 11/12/14 17:10:52

comment:2 Changed 4 years ago by jdefaria

  • Commit set to 28c70e7cd4c34339ef3ecaca94a23e4f6542f2b1
  • Status changed from new to needs_review

New commits:

28c70e7Added functionality over Number Fields with example

comment:3 Changed 4 years ago by bhutz

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

Need to fix the merge issues

comment:4 Changed 4 years ago by git

  • Commit changed from 28c70e7cd4c34339ef3ecaca94a23e4f6542f2b1 to 51e8a6a09bf115e1c659b9d8168fd0e146a9b914

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

51e8a6aFixed minor issues

comment:5 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:6 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

Need to fix merge issues. 1302 - need blank line before example 1331 - trailing white space 1323-1336: code needs optimization

The functionality appears correct.

comment:7 Changed 4 years ago by git

  • Commit changed from 51e8a6a09bf115e1c659b9d8168fd0e146a9b914 to cc5df762adfe0c413cb056ced3163fb77fa85100

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

03d36a3Fixed typos
cc5df76Merge branch 'develop' into ticket/17323

comment:8 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

Fixed minor typos and resolved merge conflicts, setting to needs_review

comment:9 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

umm..something is messed up here with the branch. You have only the last two commits. Probably something messed up with the merge. As it is just a few lines to code, it is probably best simply to re-implement them.

comment:10 Changed 4 years ago by git

  • Commit changed from cc5df762adfe0c413cb056ced3163fb77fa85100 to 459d121acc4c2729d2f7c5b5e40cc9faff0f163a

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

459d121-17323- Deleted unneeded check and cleaned up code

comment:11 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

Needs review,

comment:12 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

A couple things:

  • you still need to check if projective space so that this won't run for subschemes
  • you can streamline the check for numberfields and number field orders, but just doing the check on the fraction field.
  • I'd like to see a comment that differentiates the two methods (number field versus QQ)

comment:13 Changed 4 years ago by git

  • Commit changed from 459d121acc4c2729d2f7c5b5e40cc9faff0f163a to 937a20ee89a169d308dc3d85e647ab309b51ce0f

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

937a20e-17323- Fixed minor issues

comment:14 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:15 Changed 4 years ago by git

  • Commit changed from 937a20ee89a169d308dc3d85e647ab309b51ce0f to c792c457d7fecb941f1400e174ffeee7523afaf5

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

c792c45-17323- Fixed spelling error and circular import issue

comment:16 Changed 4 years ago by bhutz

  • Status changed from needs_review to needs_work

This if/else is causing every doctest over QQ or ZZ to fail

        if K in NumberFields() and K != QQ:
            F = copy(self)
            F.normalize_coordinates()
            return (K(F.resultant()).support())
        else:
            raise TypeError("Base Ring must be number field or number field ring")

comment:17 Changed 4 years ago by git

  • Commit changed from c792c457d7fecb941f1400e174ffeee7523afaf5 to 8e2195fcfd470d6327eddf87d3e8869aed18c450

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

8e2195f-17323- Fixed logic for error checking

comment:18 Changed 4 years ago by jdefaria

  • Status changed from needs_work to needs_review

comment:19 Changed 4 years ago by bhutz

  • Status changed from needs_review to positive_review

looks good now.

comment:20 Changed 4 years ago by vbraun

  • Status changed from positive_review to needs_work

Author name should be the real name, not trac account name

comment:21 Changed 4 years ago by jdefaria

  • Authors changed from jdefaria to Joao Alberto de Faria
  • Status changed from needs_work to positive_review

comment:22 Changed 4 years ago by jdefaria

  • Milestone changed from sage-6.4 to sage-6.6

comment:23 Changed 4 years ago by vbraun

  • Branch changed from u/jdefaria/ticket/17323 to 8e2195fcfd470d6327eddf87d3e8869aed18c450
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.