Ticket #3974 (closed defect: fixed)

Opened 3 months ago

Last modified 3 months ago

[with patch, with positive review] renaming of integral_weierstrass_model to integral_short_weierstrass_model

Reported by: wuthrich Assigned to: was
Priority: trivial Milestone: sage-3.1.2
Component: number theory Keywords:
Cc:

Description

I propose a trivial change in the name of integral_weierstrass_model. According to ell_generic.py the terminology is chosen as

Elliptic curves are always represented by `Weierstass Models' with five coefficients $[a_1,a_2,a_3,a_4,a_6]$ in standard notation. In Magma, `Weierstrass Model' means a model with a1=a2=a3=0, which is called `Short Weierstrass Model' in Sage;

so consequently the integral_weierstrass_model which gives back a Short Weierstrass Model should be called integral_short_weierstrass_model.

That is maybe pedantic and a matter of taste, but I believe it would be better.

Attachments

sage_trac3974.patch (1.8 kB) - added by wuthrich on 08/28/2008 05:10:49 AM.
sage_trac3974_new.patch (3.0 kB) - added by wuthrich on 08/29/2008 06:04:17 AM.
new patch, erplaces the first patch !

Change History

08/28/2008 05:10:49 AM changed by wuthrich

  • attachment sage_trac3974.patch added.

08/28/2008 05:16:48 AM changed by mabshoff

  • milestone set to sage-3.1.2.

Chris,

integral_weierstrass_model is part of the public API, so the rename requires that the old function is still available, but using it should throw a deprecation warning. One example is: 

def dynkin_diagram(t):
    """
    Returns the Dynkin diagram of type t.  

    Note that this function is deprecated, and that you should use DynkinDiagram 
    instead as this will be disappearing in the near future.

    EXAMPLES:
        sage: dynkin_diagram(["A", 3])
        doctest:1: DeprecationWarning: dynkin_diagram is deprecated, use DynkinDiagram instead!
        Dynkin diagram of type ['A', 3]

    """
    from sage.misc.misc import deprecation
    deprecation("dynkin_diagram is deprecated, use DynkinDiagram instead!")
    return DynkinDiagram(t)

from sage/combinat/root_system/dynkin_diagram.py

Cheers,

Michael

08/29/2008 05:09:10 AM changed by wuthrich

I shall change that.

Nevertheless, in my 3.1.1, I can't use 

from sage.misc.misc import deprecation
deprecation("dynkin_diagram is deprecated, use DynkinDiagram instead!")

instead looking at dynkin_diagram.py suggests the lines 

import warnings
warnings.warn("integral_weierstrass_model is deprecated, use integral_short_weierstrass_model instead!", DeprecationWarning, stacklevel=2)

Is this the proper way of doing ? When is the 'near future' coming ? and do I have to remove it with another patch later ?

(Thanks for the corrections and sorry for my stupid question, but it seems to me that this is the only way for me to learn these things.)

Chris.

08/29/2008 05:11:47 AM changed by wuthrich

Sorry I just found the ticket 3654 which answers my question. Chris.

08/29/2008 06:04:17 AM changed by wuthrich

  • attachment sage_trac3974_new.patch added.

new patch, erplaces the first patch !

08/29/2008 06:28:56 AM changed by wuthrich

I changed the patch according to Michael's remark. It replaces the previous patch. This patch only works with the patch from ticket 3654.

Chris.

09/01/2008 01:26:00 PM changed by cremona

  • summary changed from [with patch, needs review] renaming of integral_weierstrass_model to integral_short_weierstrass_model to [with patch, with positive review] renaming of integral_weierstrass_model to integral_short_weierstrass_model.

I approve of this, though I'm not familiar with how to do the deprecation thing. Patch applies cleanly to 3.1.2.alpha3, doctests in sage.schemes.elliptic_curves all ok.

09/02/2008 04:47:50 AM changed by mabshoff

  • status changed from new to closed.
  • resolution set to fixed.

Merged sage_trac3974_new.patch in Sage 3.1.2.alpha4