-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add getindex; replace a.coeffs[i] with a[i] #97
Conversation
I just approved this to be merged, but it seems that tests (at least in 0.5) are not passing. I'll check this again. |
return a[n+1]) | ||
|
||
getindex(a::Taylor1, n::Int) = a.coeffs[n] | ||
getindex(a::Taylor1, n::UnitRange) = a.coeffs[n] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need also methods for HomogeneousPolynomial
and TaylorN
? This may be the cause that the tests are not passing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do. I wanted to begin just with Taylor1 to get it right. I won't be able to get to the rest for a while if you want to have a go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do it then.
One question: Should we also include setindex
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, I guess we should.
setindex! Is it surprising that the Taylor1 tests pass without setindex defined? |
I guess this is because we haven't done anything like |
But I changed all a.coeffs accesses to use getindex, so I'm not sure how
any entries in a TaylorSeries are getting changed...
…On 15 Mar 2017 10:26 a.m., "Luis Benet" ***@***.***> wrote:
I guess this is because we haven't done anything like a[1] = b[1]+c[1],
with all quantities Taylor1. This could simplify things even further!
Yet, Taylor1 and the rest are immutable, so I don't know if it works
completely...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#97 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALtTtOvrZrjAg8a0PSCpiEStJKCa7j3ks5rmBE0gaJpZM4Md_PQ>
.
|
Ah I see, they're immutable so you only use the constructor. Then there's
no need or use to define setindex!?
…On 15 Mar 2017 10:29 a.m., "David P. Sanders" ***@***.***> wrote:
But I changed all a.coeffs accesses to use getindex, so I'm not sure how
any entries in a TaylorSeries are getting changed...
On 15 Mar 2017 10:26 a.m., "Luis Benet" ***@***.***> wrote:
> I guess this is because we haven't done anything like a[1] = b[1]+c[1],
> with all quantities Taylor1. This could simplify things even further!
> Yet, Taylor1 and the rest are immutable, so I don't know if it works
> completely...
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#97 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AALtTtOvrZrjAg8a0PSCpiEStJKCa7j3ks5rmBE0gaJpZM4Md_PQ>
> .
>
|
Including |
I just pushed a new commit. With this functionality, I think we can simplify even further the code. I will work on that later (on a new PR)! I'll wait to get the green lights and then merge. |
Tests are passing on Travis for 0.5 and 0.6 (on Linux). |
I agree with you. One silly question: shall I rebase and merge, or squash and merge? |
I always just use the Squash and merge button. |
Thanks a lot!! |
a.coeffs[i]
bya[i]
.getindex
andsetindex!
methods.