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.

Sign up for GitHub

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

Jump to bottom

make ReverseDiff Optional #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

make ReverseDiff Optional #13

wants to merge 1 commit into from

Conversation

jw3126
Copy link

@jw3126 jw3126 commented Oct 22, 2022

Thanks a lot for creating SNOW! ReverseDiff.jl is a heavy package, but personally I almost never actually use it.
How about making it optional?

Before PR

julia> @time using SNOW
  9.064925 seconds (30.91 M allocations: 2.149 GiB, 3.74% gc time, 0.3
 which was recompilation)

After PR

julia> @time using SNOW
  3.619427 seconds (7.36 M allocations: 549.917 MiB, 8.71% gc time, 0.96% compilation time: 3% o
f which was recompilation)

@jw3126
Copy link
Author

jw3126 commented Oct 29, 2022

@andrewning are you in principle interested in making ReverseDiff.jl optional?

@andrewning
Copy link
Member

Yeah, that's fine. Not a high priority at the moment as the startup time is a one-time cost, but when I have other updates to bundle in I'll get around to tagging a new release.

@jw3126
Copy link
Author

jw3126 commented Nov 3, 2022

Ok great!

Not a high priority at the moment as the startup time is a one-time cost

Usually yes, but in some cases, you need to restart often. E.g. when redefining structs or debugging code that hangs or crashes etc,

@jw3126
Copy link
Author

jw3126 commented Nov 13, 2022

@andrewning are you ok with merging this, even if you don't want to tag a release right away?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jw3126 @andrewning