Unit conversion can turn nonzero values into zero, result in divide-by-zero errors

Abierto
  • ¿Algo no está bien?
#400 1 comentario 0 reacciones 0 asignados Ver en GitHub

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
3/5
Tiempo estimado
1-2 días
Aptitud para principiantes
72/100
Tipo de issue
Error
Claridad
Bien especificado
Estado de actividad
Activo
Stack tecnológico
javascript
Área
backend

Línea de trabajo

Start with the comparison in src/util/units.ts at the linked lines, then find the tests for unit conversion and Divide, TruncatedDivide, and Modulo. Run the relevant tests and add coverage for the negative and positive examples in the issue. Done means the negative divisor produces the stated result without breaking the positive case or related operations.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

There is an issue with the unit conversion process that turns small negative values into zero, which turns the overall expression into a divide-by-zero error / null.

For instance:

define "DivBySmallNegative": 1 'm' div -0.00000001 'cm'

this currently throws, but the correct result should be -10000000000 '1'. The issue is specifically with negative numbers, since this returns the expected positive value correctly:

define "DivBySmallPositive": 1 'm' div 0.00000001 'cm'    // 10000000000 '1'

The sequence that causes the issue:

  1. Normalization first tries converting the divisor to m.
    -0.00000001 cm → -0.0000000001 m

  2. Unit conversion rounds that value to zero.
    convertUnit calls normalized(), which limits the decimal scale to eight places:

    -0.0000000001 → 0.00000000

  3. This new value is returned based on a comparison that doesn't work correctly for negative values:
    https://github.com/cqframework/cql-execution/blob/e8096c64ae8da45fe9a358bbf711bc36d749b696/src/util/units.ts#L132-L134
    newVal2.greaterThanOrEquals(val2)

    Here that becomes:

    0 >= -0.00000001 → true

    Consequently, it keeps meters and returns these normalized operands:

    dividend: 1 m
    divisor: 0 m

Note: I think that comparison newVal2.greaterThanOrEquals(val2) can also produce unexpected results for any negative divisor, but the impacts are most visible for these small negative numbers. The easiest fix might just be to do newVal2.abs().greaterThanOrEquals(val2.abs()) but I haven't fully tested that

This is relevant to Divide, TruncatedDivide, and Modulo.

Lenguaje dominante
JavaScript
Estrellas
85
Forks
40
Merge medio
5 d 4 h
PR fusionados (30 d)
3

Preparar el entorno

  • Sin Dockerfile ni archivo de Docker Compose
  • Tiene una plantilla de pull request
  • Sin guía de contribución

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de cqframework/cql-execution

Todos los issues de cqframework/cql-execution

Issues similares

Más issues de JavaScript

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.