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

Make index_table a Vector{Vector{UInt128}}, and add tests #86

Closed
wants to merge 1 commit into from

Conversation

lbenet
Copy link
Member

@lbenet lbenet commented Feb 19, 2017

This is a small modification addressing [#85]. It simply carries
on the hashing of the vectors of integers in UInt128 arithmetic.

@dpsanders
Copy link
Contributor

This will slow down (I would guess by rather a lot) every single calculation with the package.
It will also have its own upper bound on the number of possible variables.

I think a better solution is the one I posted in #85 (comment)

although that will require more work to make everything type stable again.

@coveralls
Copy link

coveralls commented Feb 19, 2017

Coverage Status

Coverage remained the same at 97.147% when pulling 6e27470 on index_table into 99a4d8e on master.

@lbenet
Copy link
Member Author

lbenet commented Feb 19, 2017

This will slow down (I would guess by rather a lot) every single calculation with the package.
It will also have its own upper bound on the number of possible variables.

Both statements are correct. Running perf/fateman.jl with Julia v0.5 yields almost a factor ~1.7 slower if #86 would be merged, though (interestingly) no more allocations. (Using Int128 instead of UInt128 yields essentially the same results.)

I think a better solution is the one I posted in #85 (comment)
although that will require more work to make everything type stable again.

I also agree with this. Yet, this is quite tricky, since the choice must depend on the values.

A possible approach could be to define a (global) parameter which fixes the parameterization of generate_tables{T} and in_base{T}, which eventually defines the type of index_table. Throwing an error message would also be nice, but I'm not sure how to do that.

@lbenet
Copy link
Member Author

lbenet commented Feb 20, 2017

@PerezHz The performance hit is important, and as @dpsanders states, it is not common to have situations where the used hashed numbers is so important. I suggest that you use the branch "index_table" to get access to what has been done here.

@dpsanders
Copy link
Contributor

I think we should move away from any kind of global table. Then each Taylor polynomial can store a reference to an object of the relevant type / size.

@lbenet
Copy link
Member Author

lbenet commented Feb 20, 2017

I agree that we should look for alternatives (including moving away from the global table scheme), but if such alternatives involve memory overhead, I don't think that's the way to go.

@lbenet lbenet closed this Mar 4, 2017
@lbenet lbenet deleted the index_table branch March 4, 2017 18:01
@lbenet lbenet restored the index_table branch March 4, 2017 18:01
@lbenet
Copy link
Member Author

lbenet commented Mar 4, 2017

I closed this, push -f the corresponding commits adapted to the new organization of the files (#87), and keep this open, in case it may be needed.

@lbenet lbenet reopened this Mar 4, 2017
@coveralls
Copy link

coveralls commented Mar 4, 2017

Coverage Status

Coverage remained the same at 97.579% when pulling e1c624b on index_table into 0a91842 on master.

@coveralls
Copy link

coveralls commented Mar 4, 2017

Coverage Status

Coverage remained the same at 97.579% when pulling e1c624b on index_table into 0a91842 on master.

@PerezHz
Copy link
Contributor

PerezHz commented Mar 5, 2017

Thanks @lbenet and @dpsanders for the work you are putting into this; it is helping me a lot! Perhaps a note should be added in the documentation regarding the fact that TaylorN works with at most 65 variables and if more are needed then the current solution is to use the index_table branch?

@coveralls
Copy link

coveralls commented Mar 7, 2017

Coverage Status

Coverage remained the same at 98.251% when pulling b7966f6 on index_table into 88f2fe9 on master.

@coveralls
Copy link

coveralls commented Mar 8, 2017

Coverage Status

Coverage remained the same at 98.274% when pulling 3a4d4a4 on index_table into 4a40685 on master.

@coveralls
Copy link

coveralls commented Jun 10, 2017

Coverage Status

Coverage decreased (-1.5%) to 96.552% when pulling 8766c58 on index_table into d4f0c20 on master.

@lbenet
Copy link
Member Author

lbenet commented Feb 4, 2024

Addressed by #314 and #316

@lbenet lbenet closed this Feb 4, 2024
@lbenet lbenet deleted the index_table branch February 4, 2024 03:33
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.

None yet

4 participants