[SparseArrayDOKs] Add setindex_maybe_grow! and macro @maybe_grow

[emstoudenmire]

Adds the setindex_maybe_grow! function and the macro @maybe_grow as outlined in Issue #1433.

Fixes #1433.

[emstoudenmire]

Ok just made those improvements to the @maybe_grow macro

[mtfishman]

@emstoudenmire I think we could simplify the macro implementation using a package like MacroTools.jl. There are other similar packages but I think that one is popular.

[emstoudenmire]

That package looks great – I'll give it a spin, and this has been a good "basic macro practice" for me. I was hesitant to add a new dependency for this PR but if you think we would use that package in other parts of NDTensors (probably so?) then I'll add it.

[mtfishman]

It's a lightweight and widely used package so if it simplifies the code then I don't see any harm.

[emstoudenmire]

Ok, made that change. The @capture macro is definitely nice. I read through the whole MacroTools docs and didn't so easily see a nicer way to write the part of @maybe_grow that checks it is the right type of expression.

[emstoudenmire]

There is the MacroTools.isexpr function, but the documentation for it is rather minimal so I'm not sure if it could be used.

[mtfishman]

In the Pattern Matching section it says that:

Because @capture doubles as a test as well as extracting values, you can easily handle unexpected input (try writing this by hand):
...

and then suggests syntax for catching cases when the expression is not of the correct form. It looks like @capture will output false if I'm interpreting the example they give correctly, which you can then catch and throw an error.

[emstoudenmire]

I was also hoping initially that @capture would automatically validate the expression too. Maybe it does in other cases but in this case, whenever I throw an incorrect (non-matching) expression at it I just get a very generic / cryptic error:

julia> @maybe_grow x+y+z
ERROR: MethodError: no method matching iterate(::Nothing)

(it is always that error for any expression not of the form a[I...] = v).

[mtfishman]

I was also hoping initially that @capture would automatically validate the expression too. Maybe it does in other cases but in this case, whenever I throw an incorrect (non-matching) expression at it I just get a very generic / cryptic error:

julia> @maybe_grow x+y+z
ERROR: MethodError: no method matching iterate(::Nothing)

(it is always that error for any expression not of the form a[I...] = v).

Maybe we could wrap it in a try ... catch block and throw our own error?

[emstoudenmire]

Oh, that's a good idea. I'll do that.

[emstoudenmire]

Unfortunately I am not able to get the try...catch... to work. I still get the same exception and not the one that the catch block is trying to throw. Maybe it could be because the macro is running at parse time and so the try catch block does not work there or is inactive at parse time in some sense? Here's what I tried:

macro maybe_grow(ex)
  try
    @capture(ex, array_[indices__] = value_)
    return :(setindex_maybe_grow!($(esc(array)), $value, $indices...))
  catch
    error(
      "@maybe_grow must be used with setindex! syntax (as @maybe_grow a[i,j,...] = value)"
    )
  end
end

[mtfishman]

That's too bad, I guess it requires some more investigation.

Let's at least refactor the check into a function is_siteindex!_expr(expr) is_setindex!_expr(expr) to make the macro a bit cleaner and more readable. EDIT: Fixed a typo.

[emstoudenmire]

I ended up blending the ideas of a checking function and using a try..catch block. The reason is that there are some expressions (for example @maybe_grow v) where my check itself errors, since some expressions have no .head field, so then the error message is again bad. A try..catch block ensures a good error message in all failing cases here.

Also apparently the try...catch not working when wrapped around the @capture macro seems to be due to something about MacroTools itself, versus whether try..catch is used inside a function or a macro. I say this because I also tried moving the @capture macro itself out to another function and was still not able to catch the exception that it threw.

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.52%. Comparing base (3d36e74) to head (afc5bb6).
Report is 7 commits behind head on main.

❗ Current head afc5bb6 differs from pull request most recent head 0520e67. Consider uploading reports for the commit 0520e67 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1434      +/-   ##
==========================================
- Coverage   60.83%   59.52%   -1.31%     
==========================================
  Files         147      139       -8     
  Lines        9661     9005     -656     
==========================================
- Hits         5877     5360     -517     
+ Misses       3784     3645     -139     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtfishman]

I ended up blending the ideas of a checking function and using a try..catch block. The reason is that there are some expressions (for example @maybe_grow v) where my check itself errors, since some expressions have no .head field, so then the error message is again bad. A try..catch block ensures a good error message in all failing cases here.

You should be able to catch that kind of issue in a more elegant way using dispatch by specializing on whether or not the type of the input is Expr:

function is_setindex!_expr(expr::Expr)
  return is_assignment_expr(expr) && is_getindex_expr(first(expr.args))
end
is_setindex!_expr(x) = false

is_getindex_expr(expr::Expr) = (expr.head === :ref)
is_getindex_expr(x) = false

is_assignment_expr(expr::Expr) = (expr.head === :(=))
is_assignment_expr(expr) = false

Note that I'm using === because it is a stricter check. Expr always has a head field and an args field so these checks should never error and always just output true or false.

[emstoudenmire]

That is a nicer pattern, especially since the type constraints guarantee the code inside can't error. I just updated the code to use it.

MacroTools uses a lot of similar patterns internally also.

[mtfishman]

Looks like a few more constructors need to be defined.

[mtfishman]

Thanks! This will be helpful.