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

Remove eval called by rrule_test #32

Merged
merged 9 commits into from
May 13, 2020

Conversation

sethaxen
Copy link
Member

@sethaxen sethaxen commented May 7, 2020

This fixes #30 by removing the "terrifying" eval in _make_fdm_call.

@sethaxen
Copy link
Member Author

Is there anything else this PR needs?

@oxinabox
Copy link
Member

sorry its been weekend+public holidays here since it was openned.
Will try and review monday.

src/testers.jl Show resolved Hide resolved
src/testers.jl Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
Co-authored-by: Matt.jl <matt.brzezinski@invenia.ca>
@oxinabox
Copy link
Member

oxinabox commented May 11, 2020

I am going to leave this to @mattBrzezinski to review and follow up til merged.

Copy link
Member

@mattBrzezinski mattBrzezinski left a comment

Choose a reason for hiding this comment

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

LGTM, just two minor whitespacing comments, and then making f2 definition the first thing inside of _make_fdm_call for increased readability!

src/testers.jl Show resolved Hide resolved
src/testers.jl Show resolved Hide resolved
src/testers.jl Show resolved Hide resolved
sethaxen and others added 2 commits May 11, 2020 10:25
Co-authored-by: Matt.jl <matt.brzezinski@invenia.ca>
@sethaxen
Copy link
Member Author

Thanks! Implemented the requested changes.

@nickrobinson251 nickrobinson251 added the needs version bump Need to bump the version in Project.toml before can be merged label May 12, 2020
Copy link
Contributor

@nickrobinson251 nickrobinson251 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks! A other couple tiny comments which you're free to ignore.

Before we merge, please can you bumpy the version number?
I know it's an internal-only change, but good to release this change by itself and not with other changes in case we do hit any unexpected issues

src/testers.jl Show resolved Hide resolved
src/testers.jl Show resolved Hide resolved
test/testers.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Member Author

Done!

@nickrobinson251 nickrobinson251 removed the needs version bump Need to bump the version in Project.toml before can be merged label May 13, 2020
@nickrobinson251 nickrobinson251 merged commit 2735472 into JuliaDiff:master May 13, 2020
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.

Testing functions with symbols as arguments
4 participants