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

added summary stat performance test #506

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ct-clmsn
Copy link
Contributor

@ct-clmsn ct-clmsn commented Jun 29, 2018

performance test added - accessing an array that stores indices, used for a 2nd matrix/array look up, could use some performance tuning in phylanx. this test provides a python/numpy version and the phylanx version.

@ct-clmsn ct-clmsn added this to the 0.0.1 milestone Jun 29, 2018
@ct-clmsn
Copy link
Contributor Author

this unit test represents a performance issue the white paper algorithm implementation demonstrated. it's a very simple operation but necessary for computing summary statistics.

@hkaiser
Copy link
Member

hkaiser commented Jun 29, 2018

Thanks Chris, we'll investigate.

@ct-clmsn
Copy link
Contributor Author

@hkaiser updated with another edge case - would ya'll prefer if these tests were split out across multiple files?

@hkaiser
Copy link
Member

hkaiser commented Jun 29, 2018

@ct-clmsn I think we're still fine, thanks!

@hkaiser
Copy link
Member

hkaiser commented Jul 16, 2018

@ct-clmsn we'll have to get back to this once @rtohid and @justwagle have fixed the set primitive as currently the Phylanx frontend generates broken code for your example. I'm not saying that this is the reason for the performance you're seeing, I just would like to compare apples with apples.

@ct-clmsn
Copy link
Contributor Author

@hkaiser no problem; thanks!

@hkaiser
Copy link
Member

hkaiser commented Jul 17, 2018

@ct-clmsn Actually, closer investigation shows, that the bad performance is caused by the (bad) PhySL code generated. Let's see how it will fare once the frontend etc. has been fixed.

@ct-clmsn
Copy link
Contributor Author

@hkaiser apologies on my end for not taking time to vet the PhySL layer. I'll add that to my code review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants