-
Notifications
You must be signed in to change notification settings - Fork 156
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
change binomial registration #1029
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1029 +/- ##
==========================================
- Coverage 78.32% 78.04% -0.29%
==========================================
Files 28 28
Lines 3387 3398 +11
==========================================
- Hits 2653 2652 -1
- Misses 734 746 +12 ☔ View full report in Codecov by Sentry. |
@shashi is there a reason why it's missing that dispatch? |
This seems to fix the issue for me. The problem is several-fold. First, [Real,Integer] is never actually used by Symbolics in the macro (instead the types of the arguments are used if asserted, and if not Real is used). Even if it was used, however, it wouldn't help with the issue we have as we need a dispatch that allows the second input to be a BasicSymbolic{Real} (even though a user will pass an integer via the parameter map). This is what was previously defined via @register_symbolic Base.binomial(n, k)::Int Then there is the issue that we'd basically like to just say @register_symbolic Base.binomial(n, k)::Int true
@register_symbolic Base.binomial(n, k::Int)::Int false to get all the definitions, but this leads to overwriting of previous definitions involving So the best solution I could come up with is to manually take the expressions from the second call to |
It would be better if the macro could check what methods are already defined, and only output the new subset of method definitions from the set it generates. |
Not sure why MTK tests are failing. |
006543a
into
JuliaSymbolics:master
Closes #1028.
Uses @ChrisRackauckas's suggestion on how to handle this.