Skip to content

Conversation

@polvalente
Copy link
Contributor

If we rewrite the Nx defn AST during normalization to use the number values from :math or Complex for the arity-0 versions of i/e/pi, we can benefit from the previous pull requests on precision upcasting "for free".

This is implicit behavior, so I understand if we don't want this, but I believe it makes for a better UX.

@polvalente polvalente self-assigned this Jan 27, 2025
@josevalim
Copy link
Collaborator

If the user defines their own constant, it won't have this special behaviour, right? So I think we should avoid this or have a mechanism that everyone can use, such as adding a Nx.constant or similar (maybe Nx.tensor already works like that?)

@polvalente
Copy link
Contributor Author

polvalente commented Jan 27, 2025

It actually works because either the user defines it as a module attribute, a deftransform or a defn.
In the first 2 cases, it will be passed on to a defn a number. In the latter, when composing defns, the user's constant is returned as a constant node, and everything works the same:

defmodule MyConstants do
  import Nx.Defn

  defn c, do: 1.4
end

defmodule MyModule do
  import Nx.Defn

  defn foo(x) do
    
    if x == 1 do
      {x, 1}
    else
      {MyConstants.c(), 2}
    end
  end
end

iex(9)> MyModule.foo(Nx.f64(2.0))
{#Nx.Tensor<
   f64
   1.4
 >,
 #Nx.Tensor<
   s32
   2
 >}
iex(10)> MyModule.foo(Nx.f32(2.0))
{#Nx.Tensor<
   f32
   1.399999976158142
 >,
 #Nx.Tensor<
   s32
   2
 >}

edit: Note that this defn-defined approach suffers from the same problem as regular numerical constants and Nx.Constants, where if the user returns the value first and then passes it on to a defn as an argument, it will be received as an f32.

@polvalente polvalente merged commit c6fc98d into main Jan 27, 2025
8 checks passed
@polvalente polvalente deleted the pv-feat/nx-constants-arbitrary-precision branch January 27, 2025 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants