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

Fix pattern 9 #1605

Merged
merged 3 commits into from Oct 13, 2015
Merged

Fix pattern 9 #1605

merged 3 commits into from Oct 13, 2015

Conversation

@chaosphere2112
Copy link
Contributor

@chaosphere2112 chaosphere2112 commented Oct 12, 2015

This PR substantially speeds up pattern 9 (no longer takes most of a minute!). It also refactors the patterns/hatches to be classes, which will let us do some cool stuff later down the line (like custom patterns) and makes it so the logic for fetching a specific pattern is less hardcoded (again, making it easier to add custom patterns later). Matching testdata PR is at CDAT/uvcdat-testdata#73

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Oct 12, 2015

@sankhesh You'll want to take a peek to make sure I didn't mess anything up in here.

@doutriaux1
Copy link
Contributor

@doutriaux1 doutriaux1 commented Oct 12, 2015

@chaosphere2112 darn your baseline somehow dosen't let master baseline be merged into it. I wonder why. Did you start the baseline branch a while back?

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 13, 2015

👏 : @chaosphere2112 what was the reason for the slow down?

aashish24
Copy link
Contributor

aashish24 commented on 4a7e0f6 Oct 13, 2015

the names render and draw are confusing. render and draw meant to be same in graphics domain. Can we rename render to something else?

chaosphere2112
Copy link
Contributor

chaosphere2112 commented on 4a7e0f6 Oct 13, 2015

Well, draw is a subtask of render. But, yeah, I can rename those. render and paint? Or are those still too similar?

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Oct 13, 2015

@aashish24 FillTube is kind of slow (.003 seconds per call), and Pattern 9 called it 4 times per 16 pixel square. I made it so instead of drawing it like this:

_______________
|   \    /    |
|   /    \    |
---------------

it is instead drawn like so:

_____________
|   | _ |   |
|   | _ |   |
-------------

and then rotated 45 degrees. I was able to dramatically reduce the number of draw calls (1 per column and row, and 1 per intersection) as well as using the speedier FillBox function.

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 13, 2015

thanks @chaosphere2112 so how did you trace FillTube timings? Also, thanks for changing the name from draw to paint. Actually "create" or "construct" would have better, but I am okay with paint.

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Oct 13, 2015

I just used cProfile and sorted by tottime

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 13, 2015

I just used cProfile and sorted by tottime

Thanks, it would be nice to document developer tips somewhere...

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 13, 2015

@sankhesh the changes looks good to me but you are the original author. Please review.

@chaosphere2112
Copy link
Contributor Author

@chaosphere2112 chaosphere2112 commented Oct 13, 2015

@aashish24 Documentation for that is pretty much the top google result for "profile python" 😉

@aashish24
Copy link
Contributor

@aashish24 aashish24 commented Oct 13, 2015

@aashish24 Documentation for that is pretty much the top google result for "profile python"

😆 sure, I meant in a large sense.. not just for this. We need more extensive developers documentation.

sankhesh added a commit that referenced this issue Oct 13, 2015
@sankhesh sankhesh merged commit f22289c into master Oct 13, 2015
5 of 8 checks passed
@sankhesh sankhesh deleted the fix_pattern_9 branch Oct 13, 2015
@sankhesh
Copy link
Contributor

@sankhesh sankhesh commented Oct 13, 2015

👍

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

Successfully merging this pull request may close these issues.

None yet

4 participants