Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: simplify divide and reciprocal function #1361

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

user202729
Copy link
Contributor

Motivation for this change

Fixes the bump as can be seen in https://tex.stackexchange.com/questions/611573/pgfplots-strange-bump-in-tanh-function . Probably also make divide and reciprocal faster.

(Is reliance on ε-TeX acceptable?)

Checklist

@hmenke
Copy link
Member

hmenke commented Sep 28, 2024

I had the same idea about 4 years ago. However, it's not that easy. While this change fixes your particular example, can you estimate how many old documents and other packages such a change will break? That is one of the primary reason why I try to not touch pgfmath unless it is to fix things that are currently throwing an error.

@user202729
Copy link
Contributor Author

  1. In the old code, both the input and the output must fit in a dimen as pt, otherwise error happens.
  2. Every time input and output fits in a dimen, the new code wouldn't give an error (or overflow).
  3. Since the new code only perform one operation, it would be correctly rounded.
  4. So assume some old document does not incur the bug, the only difference would be slight difference in floating point results leading to imperceptible changes. This is certainly not an issue, because other changes in LaTeX and/or popular packages have been causing changes to e.g. line breaks, spacing, hyphenation, etc. all the time (see section 5.2 and 5.3).

(that said, scaling on dimen as in your code would probably be faster than scaling on int because it eliminates the redundant operation.)

@hmenke
Copy link
Member

hmenke commented Sep 29, 2024

The new code unfortunately has a completely different rounding behavior than the old code. If you have ever done any kind of numerical science you will have painfully experienced that rounding errors accumulate and can completely change the result of a computation. If a user has a drawing with computed coordinates and those computations execute tons of divisions in the background then changing the rounding mode will change the position of these coordinates and break the drawing.

You can use this code to compare old and new results.

\documentclass{article}
\usepackage{pgfmath}
\makeatletter
\pgfmathdeclarefunction{newdivide}{2}{%
  \edef\pgfmathresult{\pgfmath@tonumber{\dimexpr
      \numexpr
      65536*\dimexpr #1pt\relax
      /\dimexpr #2pt\relax
      \relax
      sp\relax}}%
}
\makeatother 
\begin{document}
\ExplSyntaxOn
% Iterate all possible numerators and denominators from -\maxdimen to \maxdimen
\int_step_variable:nnNn {-1073741823} {1073741823} \i {
  \int_step_variable:nnNn {-1073741823} {1073741823} \j {
    % Set dimen from integer
    \dim_set:Nn \l_tmpa_dim { \i sp }
    \dim_set:Nn \l_tmpb_dim { \j sp }
    % Convert dimen to decimal (strip pt)
    \tl_set:Nx \l_tmpa_tl { \dim_to_decimal:n { \l_tmpa_dim } }
    \tl_set:Nx \l_tmpb_tl { \dim_to_decimal:n { \l_tmpb_dim } }
    % Old \pgfmathdivide
    \pgfmathdivide { \l_tmpa_tl } { \l_tmpb_tl }
    \tl_set:Nx \l_result_old_tl { \pgfmathresult }
    % Proposed new \pgfmathdivide
    \pgfmathnewdivide { \l_tmpa_tl } { \l_tmpb_tl }
    \tl_set:Nx \l_result_new_tl { \pgfmathresult }
    % Print for different results
    \tl_if_eq:NNF \l_result_old_tl \l_result_new_tl {
      \tex_message:D {
        \l_tmpa_tl / \l_tmpb_tl
        = \l_result_old_tl
        != \l_result_new_tl
        (fp_eval: \fp_eval:n { round(\l_tmpa_tl / \l_tmpb_tl, 5) })
        ^^J
      }
    }
  }
}
\ExplSyntaxOff
\end{document}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants