Skip to content

Forward mode mutable struct support - #219

Merged
oxinabox merged 23 commits into
mainfrom
ox/mutation2
Feb 1, 2024
Merged

Forward mode mutable struct support#219
oxinabox merged 23 commits into
mainfrom
ox/mutation2

Conversation

@oxinabox

@oxinaboxoxinabox commented Sep 28, 2023

Copy link
Copy Markdown
Member

@oxinaboxoxinabox self-assigned this Sep 28, 2023
@oxinaboxoxinabox changed the title Forward mode mutation supportForward mode mutable struct supportSep 28, 2023
Comment threadtest/forward_mutation.jl Outdated
Comment threadtest/forward_mutation.jl Outdated
@oxinabox

oxinabox commented Dec 29, 2023

Copy link
Copy Markdown
MemberAuthor

Changes in the ChainRulesCore PR make zero_tangent more aggressive at making structural zeros.
or NoTangents for things without tangent spaces (in particular if they don't have any fields)
This is good because it should fix up a bunch of our type inference issues, since were already using zero_tangent everywhere in this PR.
A complicating factor is we also need to fix up where we use ZeroTangent for thigns that actually have no tangent space -- which isn't wrong as such but if we don't care either way we should be disaptching on AbstractZero.
Which this PR now does.

current status is WIP i know it is broken, and will fix after the long weekend.
I suspect one bit of it being broken is we need #236

@oxinabox

Copy link
Copy Markdown
MemberAuthor

I merged #236 in here because i was running into code that was hitting the exact case that fixes in the tests.
Which does show our code path is now significantly different, probably because of the structural zeros.

Comment threadsrc/codegen/forward_demand.jl Outdated

ir = compact!(ir)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Suggested change

Comment threadsrc/stage1/recurse_fwd.jl Outdated
@oxinabox
oxinabox marked this pull request as ready for review January 22, 2024 16:49
Comment threadsrc/stage1/recurse_fwd.jl Outdated
Comment on lines +38 to +39
# It is a little dubious using StructuralTangent{B} for >1st order, but it is isomorphic.
# Just watch out for order mixing bugs.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I am not even sure it is actually dubious. Further thought has convinced me its the right thing to do.
Ironically (?) at higher order it is not a structural tangent but rather a Natural tangent.
But eitherway it follows the requirement that it can be added to its base space to get a nother value in the base space. Its like Real numbers in that way,

Comment threadtest/reverse.jl


# while
# @test times_three_while'(1.0) == 3.0 # hangs in 1.11

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

do we know why these hang? that seems bad.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

No, but its unrelated to this PR

Comment threadsrc/extra_rules.jl Outdated
Comment threadsrc/extra_rules.jl Outdated
Comment threadsrc/stage1/recurse_fwd.jl Outdated
Comment threadsrc/tangent.jl Outdated
@codecov

codecovBot commented Feb 1, 2024

Copy link
Copy Markdown

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (6f1bdf0) 55.11% compared to head (8badafd) 55.12%.

FilesPatch %Lines
src/extra_rules.jl45.45%6 Missing ⚠️
src/codegen/forward_demand.jl40.00%3 Missing ⚠️
src/stage1/forward.jl87.50%3 Missing ⚠️
src/stage1/mixed.jl0.00%3 Missing ⚠️
src/higher_fwd_rules.jl80.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #219 +/- ##
=======================================
Coverage 55.11% 55.12% =======================================
Files 28 28 Lines 2892 2917 +25 =======================================
+ Hits 1594 1608 +14 - Misses 1298 1309 +11 

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

@oxinabox
oxinabox merged commit c00ceb7 into mainFeb 1, 2024
@oxinabox
oxinabox deleted the ox/mutation2 branch February 1, 2024 12:58
Sign up for freeto 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.

Support mutation for forward mode

2 participants

@oxinabox@oscardssmith