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

Add IS_NOT_IMPLEMENTED? #334

Closed
devmotion opened this issue Apr 13, 2021 · 6 comments
Closed

Add IS_NOT_IMPLEMENTED? #334

devmotion opened this issue Apr 13, 2021 · 6 comments

Comments

@devmotion
Copy link
Member

devmotion commented Apr 13, 2021

I thought a bit about how to solve JuliaDiff/ChainRulesTestUtils.jl#135 which showed up in JuliaMath/SpecialFunctions.jl#305.

Maybe one could avoid additional keyword arguments by

  1. defining const IS_NOT_IMPLEMENTED = @thunk(error("not implemented")) in ChainRulesCore (possibly not exported), and
  2. allowing to specify x ⊢ IS_NOT_IMPLEMENTED in the tests.

I assume that 1. could be useful even without 2. since it would provide an official way for declaring incomplete differential definitions. Maybe it could even be used to solve FluxML/Zygote.jl#873 without official support for ChainRules types in Zygote by dropping ChainRules derivatives silently (i.e., returning nothing) if they are identical to IS_NOT_IMPLEMENTED (not completely sure if this is a good idea though since it might be a bit surprising).

@devmotion
Copy link
Member Author

I guess, to be able to handle it properly in Zygote it should be a singleton instead of just a constant though.

@devmotion
Copy link
Member Author

Maybe one could even avoid 2. and just skip tests automatically if a IS_NOT_IMPLEMENTED or IsNotImplemented() is returned.

@oxinabox
Copy link
Member

oxinabox commented Apr 13, 2021

I think definately a type, rather than a value.

Some other thoughts.

Could make it carry around some extra info with it.

options include:

  • A user-definable string,
  • a package name + github issue number (I like this because it would encourage it to be clearly seens as a failure
  • the source file and line number where it was defined (this could be inserted automatically with a macro like @NotImplemented()
    • DebugMode could be used to disable the extra info (replacing it with nothing), so it is super light weight.

Should it subtype AbstractZero?

If Zygote is turning it into nothing then that would achieve that.
Zygote turns all our ouf AbstractZeros into nothing.

OTOH maybe Zygote shouldn't turn it into nothing and we should perist it far enough that it becomes an error to the user if it is actually used.
(We talked about doing this for DoesNotExist before, and still might)

@mzgubic
Copy link
Member

mzgubic commented Apr 15, 2021

Overall I support this idea, thanks for raising!

Some thoughts:

  • The advantage of it being a type would be we can dispatch on it
  • I like the idea of extra info and linking to issues, perhaps a struct with one string field would work?
  • It seems misleading to subtype it as AbstractZero (since it is not a zero, it could be very colourful indeed, it's just not done/WIP)
  • Ideally, it would be kept by Zygote as NotImplemented and only error when it is used, but not otherwise. Silently changing it into nothing will probably introduce bugs.

My most optimistic guess is that by creating a struct will just work as we want it: essentially the wrap_chainrules_output will turn it into a NamedTuple which will then error if it is added to anything else, and be fine otherwise. My most pessimistic guess is that it will just break everything. I think we'l just have to try 🤷

@oxinabox
Copy link
Member

Is this something you have time to look into right now?
I am pretty busy with project stuff.

@devmotion
Copy link
Member Author

Fixed by #335.

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

No branches or pull requests

3 participants