-
Notifications
You must be signed in to change notification settings - Fork 4
Add support for 2D recommend tool #15
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
base: master
Are you sure you want to change the base?
Changes from all commits
dd2b9ca
5ecfb60
13d032a
c840a03
416f060
9397707
654681c
376ae7f
31a6665
f2aa419
ef86e4d
3619d7f
3656850
3e2155d
ac1d681
0111f3e
ed94985
4540039
df8cf7b
f8c27fe
b906e45
8a559f2
33bf700
70bbf4b
fb8fd8a
3059847
13391d3
eeff51a
822ce3a
f39c134
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,10 @@ class Advisor { | |
| Tristate::Tristate isInPlace = Tristate::TRUE, | ||
| Tristate::Tristate isReal = Tristate::TRUE, int maxSignalInc = INT_MAX, | ||
| int maxMemory = INT_MAX, | ||
| bool disallowRotation = false, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. double negation (see my comment above)
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answered in comment #15 (comment) |
||
| bool allowTransposition = false, | ||
| bool disallowSizeOptimization = false, | ||
| int countOfOptimizedDimensions = MaxNumberOfOptimizedDimensions, | ||
| bool squareOnly = false, | ||
| bool crop = false); | ||
|
|
||
|
|
@@ -37,7 +40,10 @@ class Advisor { | |
| Tristate::Tristate isInPlace = Tristate::TRUE, | ||
| Tristate::Tristate isReal = Tristate::TRUE, int maxSignalInc = INT_MAX, | ||
| int maxMemory = INT_MAX, | ||
| bool disallowRotation = false, | ||
| bool allowTransposition = false, | ||
| bool disallowSizeOptimization = false, | ||
| int countOfOptimizedDimensions = MaxNumberOfOptimizedDimensions, | ||
| bool squareOnly = false, | ||
| bool crop = false); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,12 @@ int printHelp() { | |
| "(swapping dimensions). Prohibited by default. Valid for " | ||
| "'-find' only." | ||
| << std::endl; | ||
| std::cout << "\t--disallowRotation : consider also rotation of X and Y axes " | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the difference between rotation and transposition?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fundamentally, there is no difference (both do the same - swap the sizes), however, each swap is programmed differently:
To preserve backward compatibility, I chose to add new parameter instead of reusing already used. In your opinion, is it good or bad approach? |
||
| "(swapping dimensions). Allowed by default." | ||
| << std::endl; | ||
| std::cout << "\t--disallowSizeOptimization : disable size optimization by padding/cropping. " | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If no padding / cropping is allowed, what other option is there for performance improvement, except for transposition?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is not padding/cropping, but user will still receive optimalized configuration (i.e. float, R2R, etc.). Also, I believe, there could be usecases for this type of transformation. |
||
| "Allowed by default." | ||
| << std::endl; | ||
| std::cout << "\t--squareOnly : consider only square shapes " | ||
| "(X dimension size will be used as a starting point). " | ||
| "Incompatible with --allowTransposition." | ||
|
|
@@ -61,6 +67,9 @@ int printHelp() { | |
| std::cout << "\t--maxMem MB : max memory (in MB) that transformation can " | ||
| "use, default = device limit" | ||
| << std::endl; | ||
| std::cout << "\t--countOfOptimizedDimensions COUNT : number of size dimensionsthat will be optimized, " | ||
| "Default: '3' for 3D signal, '2' for 2D signal, '1' for 1D signal" | ||
| << std::endl; | ||
| return -1; | ||
| } | ||
|
|
||
|
|
@@ -109,8 +118,11 @@ int parseRecommend(int argc, char **argv, int howMany) { | |
| howMany, parser.device, parser.x, parser.y, parser.z, parser.n, | ||
| parser.isBatched, parser.isFloat, parser.isForward, | ||
| parser.isInPlace, parser.isReal, parser.maxSignalInc, | ||
| parser.maxMemMB, parser.allowTransposition, parser.squareOnly, | ||
| parser.crop); | ||
| parser.maxMemMB, parser.disallowRotation, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a design flaw of the original code.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can fix it, however, I would prefer to fix it in different pull request as I dont want to combine pull requests with different focus (adding functionality vs. refactoring). Is it okay? |
||
| parser.allowTransposition, | ||
| parser.disallowSizeOptimization, | ||
| parser.countOfOptimizedDimensions, | ||
| parser.squareOnly, parser.crop); | ||
|
|
||
| cuFFTAdvisor::Transform::printHeader(stdout); | ||
| std::cout << std::endl; | ||
|
|
@@ -142,8 +154,10 @@ int parseFind(int argc, char **argv, int howMany) { | |
| parser.isFloat, parser.isForward, | ||
| parser.isInPlace, parser.isReal, | ||
| parser.maxSignalInc, parser.maxMemMB, | ||
| parser.allowTransposition, parser.squareOnly, | ||
| parser.crop); | ||
| parser.disallowRotation, parser.allowTransposition, | ||
| parser.disallowSizeOptimization, | ||
| parser.countOfOptimizedDimensions, | ||
| parser.squareOnly, parser.crop); | ||
|
|
||
| cuFFTAdvisor::BenchmarkResult::printHeader(stdout); | ||
| std::cout << std::endl; | ||
|
|
||
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.
personally I'm not a fan of negated bool variables. I would prefer
allowRotationandallowSizeOptimization(multiple occurrences).This would also require changes in the
inputParserandmain.cppThere 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 understand your reasoning. Why did I choose to use negated bool variable is because I wanted "rotation" to be enabled by default in the cuFFTAdvisor. Same goes for disallowSizeOptimization (new option to disable cropping/padding). I think, it is a nice addition for the tool, however, as size change is the main focus, the only possibility to add this option is by negating it as "disallow" and having SizeOptimization enabled until user chooses to disable it.