Destructuring/iterating a `Tangent` does not unthunk fields, unlike `getindex`/`getproperty`

Abierto
  • ¿Algo no está bien?
#712 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
4/5
Tiempo estimado
3-5 días
Aptitud para principiantes
28/100
Tipo de issue
Error
Claridad
Bastante claro
Estado de actividad
Activo
Área
devtools

Línea de trabajo

Start in src/tangent_types/structural_tangent.jl, comparing the unthunk calls in getindex and getproperty with the Base.iterate and Base.indexed_iterate forwarding. Reproduce the snippet with collect(t) and first(nt) to see the thunks. Done means the maintainers have chosen between unthunking iteration and documenting indexed access, and the chosen option is implemented or documented with tests. The design choice is still open, so confirm it in the thread before writing code.

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

Descripción

Field access on a Tangent unthunks the field, but iteration does not:

julia> using ChainRulesCore

julia> t = Tangent{Tuple{Float64,Vector{Float64}}}(1.0, @thunk([2.0]));

julia> t[2]          # Vector{Float64}

julia> _, b = t; b   # Thunk

julia> collect(t)    # [1.0, Thunk]

julia> nt = Tangent{NamedTuple{(:a,),Tuple{Vector{Float64}}}}(a=@thunk([1.0]));

julia> nt.a          # Vector{Float64}

julia> (; a) = nt; a # Vector{Float64}

julia> first(nt)     # Thunk

getindex/getproperty call unthunk (src/tangent_types/structural_tangent.jl, since #121), whereas Base.iterate and Base.indexed_iterate forward to the backing without unthunking.

This caused a bug in ChainRules (JuliaDiff/ChainRules.jl#843): the findnz pullbacks did _, _, V̄ = unthunk(Δ). With Zygote, Δ is Tangent{Any}((ZeroTangent(), ZeroTangent(), thunk)), so V̄ was still a thunk and sparse(I, J, V̄, m, n) threw a MethodError.

There are two possible resolutions:

  1. Make iterate/indexed_iterate unthunk, for consistency with getindex/getproperty. This has a downside: positional destructuring would then also compute components the rule discards, e.g. the _ in _, _, V̄ = Δ.
  2. Keep the current behaviour, but document that rules should access the fields they need via getindex/getproperty (or property destructuring (; a) = Δ) and not via positional destructuring or iteration.

Option 2 seems preferable for performance, but the current behaviour is a silent footgun either way. ChainRules.jl#843 now uses indexing (unthunk(Δ)[3]).

Lenguaje dominante
Julia
Estrellas
267
Forks
66
Merge medio
1 d 1 h
PR fusionados (30 d)
9

Preparar el entorno

Este proyecto no incluye contenedor de desarrollo, Dockerfile ni guía de contribución, así que la configuración corre por tu cuenta: empieza por su README y consulta nuestra guía para la primera contribución para los pasos generales.

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 JuliaDiff/ChainRulesCore.jl

Todos los issues de JuliaDiff/ChainRulesCore.jl

Issues similares

Más issues de Julia

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.