HomePhorge

Documented it a bit handled the error and made it into an Abstraction
Concern Raised9c74792e6274

Referenced Files
None
Subscribers

Description

Documented it a bit handled the error and made it into an Abstraction

Furthermore it now need abstractions instead of a specific abstract function.

Details

Auditors
btutzer
Provenance
danielAuthored on May 3 2019, 11:04 AM
dschnoellPushed on May 3 2019, 11:04 AM
Parents
R20:64f37e32bf4f: Merge remote-tracking branch 'origin/feature/linear_functions' into CrossRel…
Branches
Unknown
Tags
Unknown

Event Timeline

btutzer subscribed.

Your probably going to hate me after reading all my inline comments...
Lots of them are just spell-checks. I don't care about comments, but please try to keep the documentation correct. I myself am now looking into installing a spell-checker plugin for my IDE.

/include/rosa/agent/CrossReliability.h
44

nitpick: there -> their (applies to all appearances of there in this file except the first).

47

*combinations
*corresponding

48

*functions

57

Have you thought about using a smart pointer here? As they are commonly used throughout the library, I would suggest to use them here as well for consistency (and obviously all other perks that come with smart pointers).

72

*keep

73

I suggest sacrificing conformity with maxis code in favor of readability/performance. I think something like

Type_t diff = A-B;
return diff > 0 ? diff : -diff;

or

return std::abs(A-B);

would be better here.

But this is just nitpicking again. This function is obviously readable enough, I just want to make a point that the reason we are reimplementing is to do it better than maxi ;)

125

Heisenberg would approve

134

*Calculates

142

Why is the operator() not implemented? Is that done somewhere else? Am I missing something?

146

Could fuzzyAND from include/rosa/support/math.hpp be used here?

152

If you use fuzzyAND and fuzzyOR, I think you should move this to include/rosa/support/math as well, so we have a collection of combination-methods there.

158

Could fuzzyOR from include/rosa/support/math.hpp be used here?

203

Most comments from CrossReliability (including spell-checking) also apply here.

229

Why define this twice? Couldn't a single function be used both for CrossReliability and CrossConfidence?
At this point, I would suggest deleting this function completely and use std::abs(A-B) wherever it appears. You could use

using std::abs;

and then just write abs(A-B) which reads as absolute-difference, which is exactly what it does. I think it does not get much more readable than that.
The only disadvantage that I see is that the assert would be missing, but std::abs is only defined for arithmetic types so that would be checked at compile time as well.

248

*evaluates

This commit now has outstanding concerns.Jun 25 2019, 9:42 AM