VisionEval / VisionEval-Dev

Development version of VisionEval framework
https://visioneval.github.io/
Apache License 2.0
6 stars 32 forks source link

Updated logical operators for R 4.2.0 #189

Closed srosofsky-volpe closed 1 year ago

srosofsky-volpe commented 2 years ago

Replaced instances where '||' logical operators were used to '|' logical operators within an 'any()' function.

dflynn-volpe commented 2 years ago

Evaluating to see if this solves #181

jrawbits commented 2 years ago

Thanks for the pull request and the review request. Just looking at the code, I think this may be heading in the wrong direction.

There's nothing wrong with doing if tests such as if ( object1 || object2 ) (using the double vertical bar if we know that the two objects have a length of one - the first couple of changes proposed were conditions where that was always the case (e.g. length returns a scalar value and if we test it for greater than zero, it's the scalar boolean we need). There's nothing wrong with the replacement code (which would work) - it's just overkill unless we know we're comparing vectors item-by-item.

There are in a number of the modules where the problem goes the other way: the single vertical bar or single ampersand (element by element combination of two logical vectors ) is used on things that are of length one - in those cases, wrapping them in "any" (or "all" if it is an ampersand operator) as was done here could backstop the immediate problem without forcing us to ask whether the operands are scalar or vector. As above, simpler code would use the double bar or double ampersand in the if test, but using "any" or "all" is a neat way to make it not matter what is actually being checked.

One potential "gotcha" with the any/all approach is that the module tests really are just looking at the first element (that was the old documented behavior of the if or while tests), and if there are additional elements, we might end up with different results if the some of the subsequent vector elements (which used to be ignored) are inconsistent. I'm not sure if that will be a problem in practice.

jrawbits commented 1 year ago

I'm closing this, since the essential items have been merged over into the larger 'r42' pull request.