Shakeb #11
Shakeb #11
Conversation
liboptv/src/parameters.c
Outdated
| if ( fscanf(par_file, "%d\n", &(ret->seq_first)) == 0 | ||
| || fscanf(par_file, "%d\n", &(ret->seq_last)) == 0 | ||
| || fscanf(par_file, "%d\n", &(ret->max_shaking_points)) == 0 | ||
| || fscanf(par_file, "%d\n", &(ret->max_shaking_frames)) == 0 ) |
There was a problem hiding this comment.
If we're going through the trouble of all the 'or's then we don't need a goto.
There was a problem hiding this comment.
My logic is: If one of 'fcanf's returns 0 then the parameter was not read and it is considered an error.
What do I miss here?
There was a problem hiding this comment.
Of course. My problem is not twith the or, it's with the goto. The goto is there to avoid nesting, but with the short-circuit 'or' you don't nest so much, so you can do a simple if/else.
|
parameters.pyx actually contains a bunch of changes to the multimedia stuff. |
|
These are some minor changes in constructor's signature. Should I commit On Sat, Jun 13, 2015 at 8:25 PM, Yosef Meller [email protected]
|
|
Yes, drop them from this PR, you can propose a general janitoring PR which includes this, later. |
|
|
||
| # now we have two identical ShakingParams objects (sp1 and sp2) for comparison | ||
| self.assertTrue(sp1==sp2) | ||
| self.assertFalse(sp1!=sp2) |
There was a problem hiding this comment.
What's the difference between this line and the one before? Same for the following occurrence on lines 59-60.
There was a problem hiding this comment.
The comparison is performed through function defined in ShakingParams:
def __richcmp__ (ShakingParams self, ShakingParams other, operator): The third parameter is integer value to indicate the operation performed: 2 for '=' , 3 for '!=' (you must refer to them separately).
For more info: http://docs.cython.org/src/userguide/special_methods.html#rich-comparisons
So in order to check the comparisons I must pass '==' as well as '!=' .
There was a problem hiding this comment.
I see. This is not trivial, better add a comment explaining it.
|
Other than the line comments above, looks solid. |
…to testing_fodder in liboptv/tests Add changes and corrections in response to Yosef's comments on pull request OpenPTV#11
These changes will be commited in a relevant branch.
|
Same comment here on the testing_fodder directory: take only what you need to survive. |
|
Other than that, PR seems fine. Fix it and I'll do a PR to upstream. |
Some improvements to multimedia code
|
Were all issues resolved in this PR or changes are still needed? |
|
I think this PR starts from some different point in history - I cannot test it anymore using
|
Shake parameters additions, changes, bindings and tests.