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 the jacobian and primal_value primitives #95

Merged
merged 1 commit into from
Sep 19, 2023
Merged

Conversation

devmotion
Copy link
Member

The jacobian and the primal_value support for @primitive does not provide any benefit but IMO rather makes the code less readable (related also to #91): The macro version is equivalent to directly defining AD.jacobian or AD.primal_value.

Hence this PR proposes to remove support for @primitive function jacobian and @primitive function primal_value (which seems unused and untested).

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.74% 🎉

Comparison is base (041d760) 84.25% compared to head (2b321b0) 85.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #95      +/-   ##
==========================================
+ Coverage   84.25%   85.00%   +0.74%     
==========================================
  Files           8        8              
  Lines         470      460      -10     
==========================================
- Hits          396      391       -5     
+ Misses         74       69       -5     
Files Changed Coverage Δ
src/AbstractDifferentiation.jl 80.51% <ø> (+0.85%) ⬆️
ext/AbstractDifferentiationFiniteDifferencesExt.jl 100.00% <100.00%> (ø)
ext/AbstractDifferentiationReverseDiffExt.jl 100.00% <100.00%> (ø)

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

@oxinabox
Copy link
Member

we should do this.
I guess it is technically breaking but since noone is using them, probably fine

@gdalle
Copy link
Member

gdalle commented Sep 19, 2023

PR #93 is also breaking, and I'd like to break more stuff by tackling #53, so who cares? We can just tag 0.6 once we've done all that

@mohamed82008
Copy link
Member

I am in favour of breaking changes in this package and tagging a new release. I apologise for not being too actively involved here. Please tag team on this package without me and freely merge PRs if you all agree on the changes. I don't want to get in the way of progress here. Good luck!

@devmotion
Copy link
Member Author

Could someone officially approve the PR? 🙂

@mohamed82008 mohamed82008 merged commit b307fea into master Sep 19, 2023
@devmotion
Copy link
Member Author

Thank you @mohamed82008! Makes me wonder though if we should adopt the colprac guidelines if more people are maintaining the package now (so far my impression was that basically everything should be approved by you 🙂)?

@devmotion devmotion deleted the dw/primitive branch September 19, 2023 18:58
@mohamed82008
Copy link
Member

mohamed82008 commented Sep 19, 2023

As the initiator of this package, I played the role of its maintainer for a while and then I got busy with other stuff and the package was not actively maintained. I think this package should be community-maintained since there seems to be enough interest and no time on my part to be the maintainer. I am not familiar with colprac guidelines for multiple maintainers. But I would say as long as 1 qualified reviewer approves the PR and no one objects in a certain time window, it can be safely merged. Happy to adopt any other standard folks are used to though.

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