Skip to content

Conversation

@devmotion
Copy link
Member

@devmotion devmotion commented Apr 20, 2021

Currently tests fail since check_add!! errors for Composites with a field of type NotImplemented: the implementation of + for Composite in ChainRulesCore uses elementwise_add which is implemented with + but + is not defined for NotImplemented.

My suggestion would be to allow operations with Composites with fields of type NotImplemented similar to how muladd is enabled for NotImplemented, i.e., NotImplemented wins always, but to not allow general calculations with +. This could be achieved by defining

_add(x, y) = x + y
_add(x::NotImplemented, y) = x
_add(::NotImplemented, y::NotImplemented) = y
_add(x::NotImplemented, ::NotImplemented) = x

and using _add instead of + in elementwise_add. I already confirmed that this would fix the test errors.

@devmotion
Copy link
Member Author

Hmm just thought, maybe one should use @test_broken false instead of @test true for NotImplemented to indicate more clearly that something should be fixed?

@mzgubic
Copy link
Member

mzgubic commented Apr 20, 2021

Nice, thanks for adding this here as well. I would support the use of @test_broken false as you suggest (and maybe add a comment to the testset in this package that a broken test is expected).

With regards to the failing Composite tests: Should we just add + method to NotImplemented as discussed in the first PR? It does seem relatively harmless and would avoid the suggested workaround. Sorry for anticipating this when I was reviewing the previous PR!

@mzgubic
Copy link
Member

mzgubic commented Apr 26, 2021

Thanks for adding the update. I think it is good to go, but I do have a suggestion for an improvement. If you are busy with other things I can open an issue we can tackle it separately.

We use meta testing tools to test for errors:

function errors(f, msg_pattern="")

Perhaps we could add a similar thing that would catch broken tests. This way, the current tests for check_equal would show up as passes.

EDIT: probably best to tackle this separately

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.
I have made some comments, but merge when you are happy.

@devmotion devmotion merged commit 53bd8ff into master Apr 27, 2021
@devmotion devmotion deleted the dw/notimplemented branch April 27, 2021 13:48
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.

4 participants