Code

Opened 8 years ago

Closed 7 years ago

#2365 closed defect (fixed)

[patch] models.FloatField should be renamed

Reported by: adurdin@… Owned by: adrian
Component: Database layer (models, ORM) Version:
Severity: normal Keywords:
Cc: rudolph.froger@…, gary.wilson@…, sergey.kirillov@…, andreas.eigenmann@…, nirvdrum@…, frederic.roland@…, waylan@…, adurdin@…, sandro@…, djangotrac@…, dan.fairs@…, simoncelen@…, dcramer@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The behaviour of models.FloatField is inconsistent with its name. In the database backends it is implemented as a fixed-point NUMERIC column, and in Python it returns values of type decimal.Decimal -- which are compatible with each other, but are both incompatible with floating-point numbers. So it should be renamed to DecimalField or something similar that doesn't imply floating-point numbers.

Attachments (3)

DecimalField.diff (11.7 KB) - added by adurdin@… 8 years ago.
Patch, also renames forms.FloatField (see #2366)
DecimalField-FloatField.diff (17.1 KB) - added by adurdin@… 8 years ago.
Separate DecimalField and FloatField db and form fields
DecimalField-FloatField.2.diff (143.4 KB) - added by adurdin@… 7 years ago.
Patch -- implements database FloatField and DecimalField, and supporting fields in oldforms and newforms, with tests.

Download all attachments as: .zip

Change History (30)

Changed 8 years ago by adurdin@…

Patch, also renames forms.FloatField (see #2366)

comment:1 Changed 8 years ago by anonymous

  • Summary changed from models.FloatField should be renamed to [patch] models.FloatField should be renamed

comment:2 Changed 8 years ago by Jorge Gajon <gajon@…>

When you are using MySQL as your database backend, you get back Decimal instances for FloatField attributes like you described. But when your database backend is SQLite you get back normal python float numbers. I don't know what happens with PostreSQL

This is inconsistent behavior that should probably be addressed first.

comment:3 Changed 8 years ago by adurdin@…

There should definitely be a DecimalField that has nothing to do with floats -- probably also a FloatField for floating-point numbers, if there's a good use case for it.

Changed 8 years ago by adurdin@…

Separate DecimalField and FloatField db and form fields

comment:4 Changed 8 years ago by adurdin@…

New patch: this creates a separate models.DecimalField and models.FloatField, and similar forms.DecimalField and forms.FloatField, and finally, validators.IsValidDecimal and validators.IsValidFloat.

Motivation for this separation comes from the fact that floats lack accuracy to represent decimals (e.g. for monetary values), and decimals are a bad match for floats (e.g. for values with large magnitude exponents). Following this motivation, IsValidDecimal does not accept numbers with a literal exponent, although IsValidFloat does.

This patch also modifies the MySQL and Sqlite database wrappers (I couldn't test against the others), to map DecimalField to NUMERIC, and FloatField to DOUBLE (MySQL) or NUMERIC (Sqlite, which means it's really a string, only collated as a number). The conversions table for MySQL is also changed so that DECIMAL and NEWDECIMAL fields are always returned as strings -- as the default behaviour is to return decimals if possible, or else floats; and we never want a float where we expected a decimal.

Finally, this patch should work without the decimal module: if the decimal module is available, then models.DecimalField attributes will be decimal.Decimal instances; if not, then they will be strings. If the user needs to perform arithmetic, then he can call float() and operate within the accuracy limits of floats, but it's safer not to convert implicitly.

comment:5 Changed 8 years ago by mtredinnick

I don't like the implication of the last paragraph of the previous comment. It means that every single time you do arithmetic with this field, you need to either test that you are running on Python 2.4 or call float(). This is a huge burden on the developer. It has to work a bit more transparently than that.

comment:6 Changed 8 years ago by anonymous

What about simply distribuite the module decimal.py in django.utils,
as we already do for, i.e., threading?

try:
   # Only exists in Python 2.4+
   from decimal import Decimal
except ImportError:
   # Import copy of _decimal.py from Python 2.4
   from django.utils._decimal import Decimal

Decimal is just a single python file. See
http://www.taniquetil.com.ar/facundo/bdvfiles/get_decimal.html#downloading-the-files-separately

comment:7 Changed 8 years ago by Esaj

+1 on this. It would be more consistent to map NUMERIC to decimal than to float. We should distribute decimal.py to maintain compatibility with Python 2.3.

comment:8 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Cc gary.wilson@… added

comment:9 Changed 7 years ago by Sergey <rushman@…>

  • Cc sergey.kirillov@… added

comment:10 Changed 7 years ago by Sergey <rushman@…>

+1 on distributing decimal.py with Django

comment:11 Changed 7 years ago by andreas.eigenmann@…

  • Cc andreas.eigenmann@… added

+1 on this. A decimal type is very important for dealing with money.

comment:12 Changed 7 years ago by pupeno@…

+1 on
1) Renaming FloatField to DecimalField (and make it consistently Decimal across databases).
2) Creating a new FloatField that is indeed a floating point value (according to what databases provide).

I would say 1 is essential, 2 is nice to have.

comment:13 Changed 7 years ago by anonymous

  • Cc nirvdrum@… added

comment:14 Changed 7 years ago by anonymous

  • Cc frederic.roland@… added

comment:15 Changed 7 years ago by waylan@…

  • Cc waylan@… added

comment:16 Changed 7 years ago by adurdin@…

  • Cc adurdin@… added

Changed 7 years ago by adurdin@…

Patch -- implements database FloatField and DecimalField, and supporting fields in oldforms and newforms, with tests.

comment:17 Changed 7 years ago by adurdin@…

Added an updated patch against r4439.

  • db.FloatField handle only floats, and db.DecimalField handle only decimals.
  • oldforms.FloatField and oldforms.DecimalField are the defaults for the db fields and do the Right Thing validation-wise
  • newforms.FloatField and newforms.DecimalField ditto; but they also support max_value and min_value arguments à la newforms.IntegerField

This patch includes tests for both the database fields and the form fields.

comment:18 Changed 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

Haven't heard from a core developer yet regarding this, so I'll leave it in their hands as to what we're going to do with this issue.

comment:19 Changed 7 years ago by anonymous

  • Cc sandro@… added

comment:20 Changed 7 years ago by Jon Colverson <djangotrac@…>

  • Cc djangotrac@… added

comment:21 Changed 7 years ago by Dan Fairs <dan.fairs@…>

  • Cc dan.fairs@… added

comment:22 Changed 7 years ago by simonbun <simonbun@…>

  • Cc simoncelen@… added

comment:23 Changed 7 years ago by ubernostrum

#4211 was closed in favor of this.

comment:24 Changed 7 years ago by David Cramer <dcramer@…>

  • Cc dcramer@… added

comment:25 Changed 7 years ago by Van.Lepthien@…

  • PostgreSql behavior seems to be analogous to that of MySQL. Using FloatField worked with SQLite for floating point values, but 'broke' (i.e., worked properly) when we tried to insert a floating point number into a NUMERIC field.
  • SQLite behavior occurs because Sqlite puts whatever value it's passed into the database if it can't do a conversion to the defined type - field types in the table definitions are hints, but they are not strictly enforced. See http://www.sqlite.org/faq.html#q3. It's a feature.
  • We are developing a system that requires the use of floating point numbers in the database. We probably have to use this patch (which is preferable to an ugly workaround) in order to move forward on our project.

comment:26 Changed 7 years ago by anonymous

  • Cc rudolph.froger@… added

comment:27 Changed 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [5302]) Fixed #2365, #3324 -- Renamed FloatField to DecimalField and changed the code
to return Decimal instances in Python for this field. Backwards incompatible
change.

Added a real FloatField (stores floats in the database) and support for
FloatField and DecimalField in newforms (analogous to IntegerField).

Included decimal.py module (as django.utils._decimal) from Python 2.4. This is
license compatible with Django and included for Python 2.3 compatibility only.

Large portions of this work are based on patches from Andy Durdin and Jorge
Gajon.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.