Add support for 2D recommend tool #15
Conversation
…into 2D-advisor
DStrelak
left a comment
DStrelak
left a comment
There was a problem hiding this comment.
Can you please give me an example where these changes produce a better result than the original code (I have cuda 12.6, if it matters)?
| Tristate::Tristate isForward, Tristate::Tristate isInPlace, | ||
| Tristate::Tristate isReal, int maxSignalInc, int maxMemory, | ||
| bool allowTransposition, bool squareOnly, bool crop) { | ||
| bool disallowRotation, bool allowTransposition, bool disallowSizeOptimization, |
There was a problem hiding this comment.
personally I'm not a fan of negated bool variables. I would prefer allowRotation and allowSizeOptimization (multiple occurrences).
This would also require changes in the inputParser and main.cpp
There was a problem hiding this comment.
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.
| Tristate::Tristate isInPlace = Tristate::TRUE, | ||
| Tristate::Tristate isReal = Tristate::TRUE, int maxSignalInc = INT_MAX, | ||
| int maxMemory = INT_MAX, | ||
| bool disallowRotation = false, |
There was a problem hiding this comment.
double negation (see my comment above)
| in.Y = in.originalX; | ||
| } | ||
|
|
||
| bool SizeOptimizer::swapSizes2D(GeneralTransform &in, const Polynom &x, const Polynom &y) { |
There was a problem hiding this comment.
I'm not very happy about this method.
Please add documentation to the header file.
Also, the if block starting at line 74 is very long and hard to read and reason about.
Consider refactoring it by using a single decision variable for each case, e.g.:
const bool case1 = (!divisibleBy2X) && (kernelCountX <= 1.5) && (in.X <= in.Y) && (!divisibleBy2Y); // some comment explaining why are we checking these values, possibly referencing your master theses
...
if (case1 || case 17) { swapSizes(in);}
There was a problem hiding this comment.
I have added documentation for swapSizes2D function and final decision tree picture to this repository. Also, I have made a little refactoring related to dead end branches, so the IF is a little shorter.
However, I would like to keep the structure of IF as it is. I agree that it is very long and without context, it is also hard to understand. However, with context (such as the decision tree picture), it is, in my opinion, clearer to see how this method make a decision whether to swap or not, as developer see the whole decision pipeline.
If you dont agree, I can refactor it as you suggested.
…into 2D-advisor
2 participants
New features: