Rules with Mooncake #240
Rules with Mooncake #240
Conversation
Codecov Report❌ Patch coverage is
🚀 New features to boost your workflow:
|
|
Should help with #237 |
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Nice work, looks like things are coming together!
We can go over it in a bit more detail in the coming week, but I already have some global comments.
First, I think this is missing the allocator argument right now, which will typically be filled in by the macro calls. Since this shouldn't be differentiated that's not too hard to add, but there are some interesting questions surrounding how to handle how temporaries are dealt with.
For example, I know that for chain rules we effectively have to disable marking intermediates as temporary, which we don't need to do here, but there might be some complications if the allocator is a preallocated buffer for example?
A secondary question is just about code organizing, where all of the indexing logic is now duplicated in 3 places- chain rules, mooncake rules and TensorKit rules. There might be some argument for defining pullbacks that can be shared, similar to how matrixalgebrakit handles this. (However, as I'm typing this, I also don't think we should block this progress because of that)
|
Also, would you mind splitting of the minimal supported version increase in a separate PR? I think this is a good change in any case, but then I can also update the branch protection rules separately |
I agree with the conclusion here but (you're probably tired of hearing this) we should open an issue. I tried to move some of the logic into the |
|
|
@lkdvos is this then good to go? |
|
Can I take a quick look? Maybe a good way to refresh my Mooncake understanding. |
Please do! We have freedom of information on this PR :) |
| if Tα == Zero && Tβ == Zero | ||
| scale!(dC, zero(TC)) | ||
| return ntuple(i -> NoRData(), 12) | ||
| end |
There was a problem hiding this comment.
Is it worth special casing this? Without this, I think you still get the correct result, only less efficiently. But this case should probably never happen anyway.
There was a problem hiding this comment.
I agree it should not happen but in case it does, why not exit early and save some cycles?
|
Great PR, I still find Mooncake easier to read than Enzyme. |
|
Good te merge for me (if CI passes) |
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Reimplements many of the rules (I couldn't get the automated importer to work well) using Mooncake. Tests currently do not cover complex elements as I need to make a quick PR to StridedViews first, once that is done, I will enable the tests here.