HomePhorge
Diffusion SoC_Rosa_repo a94537b537bb

Implemented RangeConfidence
Auditeda94537b537bb
Actions

Description

Implemented RangeConfidence

Details

Auditors
• juhasz
goetzinger
Provenance
• btutzerAuthored on Apr 26 2019, 1:07 PM
• btutzerPushed on Apr 26 2019, 1:07 PM
Parents
R20:4cca02b7e4af: Make RoSA build with latest MSVC (Visual Studio 2019)
Branches
Unknown
Tags
Unknown
Tasks
Restricted Maniphest Task

Event Timeline

• btutzer committed R20:a94537b537bb: Implemented RangeConfidence (authored by • btutzer).Apr 26 2019, 1:07 PM
• btutzer added a task: Restricted Maniphest Task.Apr 26 2019, 1:29 PM
• btutzer added auditors: • juhasz, goetzinger.
This commit now requires audit.Apr 26 2019, 1:29 PM
• juhasz raised a concern with this commit.Apr 26 2019, 6:29 PM
• juhasz subscribed.

Raw pointers are used in PartialFunction::RA, but RangeAbstraction destructs the stored values directly (i.e., the pointers only in this case). This causes memory leak.

I do not see immediately if there is a need for using pointers there... I would consider storing the values directly instead of pointers.
If using pointers is motivated, some kind of smart pointer would work better (e.g., std::shared_ptr).

This commit now has outstanding concerns.Apr 26 2019, 6:29 PM
• btutzer added a comment.Apr 27 2019, 10:48 AM
In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8364, @juhasz wrote:

Raw pointers are used in PartialFunction::RA, but RangeAbstraction destructs the stored values directly (i.e., the pointers only in this case). This causes memory leak.

I do not see immediately if there is a need for using pointers there... I would consider storing the values directly instead of pointers.
If using pointers is motivated, some kind of smart pointer would work better (e.g., std::shared_ptr).

Storing the values as Abstractions would slice of any specialization, so If one would pass LinearFunctions the PartialFunction would evaluate them as Abstractions, not as LinearFunctions, therefore only returning the default value. Pointers are definitely necessary, but I agree that smart pointers would be better.
I though about implementing a destructor that would delete the objects, but then a pre-condition to the constructor would be necessary, forcing the passed pointers to be owned by the PartialFunction.
I agree that smart-pointers are the best option here and will implement them.

• juhasz added a comment.Apr 27 2019, 12:27 PM

Storing the values as Abstractions would slice of any specialization,

Of course, I missed that detail... Then we agree about smart pointers.

• btutzer mentioned this in R20:7d93f54bc66c: Moved from raw pointers to std::shared_ptr's..Apr 30 2019, 10:17 AM
• btutzer requested verification of this commit.May 2 2019, 4:39 PM

Addressed in 7d93f54bc66c

This commit now requires verification by auditors.May 2 2019, 4:39 PM
• juhasz accepted this commit.May 2 2019, 4:56 PM
goetzinger raised a concern with this commit.May 3 2019, 11:02 PM
goetzinger subscribed.

Hello Benedikt, I checked Abstraction.hpp, agent-functionalities.cpp and functionAbstraction.hpp. I haven't seen any errors but I don't understand the whole code. If you have time to skype with me a bit in the next days, it would be nice. Br Maxi

This commit now has outstanding concerns.May 3 2019, 11:02 PM
• btutzer requested verification of this commit.May 4 2019, 10:37 AM
In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8669, @goetzinger wrote:

Hello Benedikt, I checked Abstraction.hpp, agent-functionalities.cpp and functionAbstraction.hpp. I haven't seen any errors but I don't understand the whole code. If you have time to skype with me a bit in the next days, it would be nice. Br Maxi

Hi, How about monday 10 am (11am FIN)?

This commit now requires verification by auditors.May 4 2019, 10:37 AM
goetzinger added a comment.May 4 2019, 11:00 AM
In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8679, @btutzer wrote:
In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8669, @goetzinger wrote:

Hello Benedikt, I checked Abstraction.hpp, agent-functionalities.cpp and functionAbstraction.hpp. I haven't seen any errors but I don't understand the whole code. If you have time to skype with me a bit in the next days, it would be nice. Br Maxi

Hi, How about monday 10 am (11am FIN)?

Fine for me.

• btutzer added a comment.May 4 2019, 11:50 AM

! In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8669, @goetzinger wrote:
Hello Benedikt, I checked Abstraction.hpp, agent-functionalities.cpp and functionAbstraction.hpp. I haven't seen any errors but I don't understand the whole code. If you have time to skype with me a bit in the next days, it would be nice. Br Maxi

Also, please note that I updated some comments yesterday (after our call), so make sure you pulled the latest commit from the feature/linear_functions branch (1d3bf7fe7a40), maybe the new comments clear some things up.

goetzinger accepted this commit.May 12 2019, 10:35 PM

I checked the files. Seems good to me.

The only questions I have:

  1. With which rule did you give these typename names? For example, "template <typename T, typename A> class LinearFunction :". Why not something more descriptive instead of "T" and "A"?
  2. Why did you use all the time this operator overloading function. Could it not by also done with a simple function that can be called and returns the result?
All concerns with this commit have now been addressed.May 12 2019, 10:35 PM
• btutzer added a comment.May 13 2019, 11:27 AM
In R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8710, @goetzinger wrote:

The only questions I have:

  1. With which rule did you give these typename names? For example, "template <typename T, typename A> class LinearFunction :". Why not something more descriptive instead of "T" and "A"?

T and A are very common for Type names. I agree that they are not very descriptive though. For the RangeConfidence i switched to D (Domain-Type), I (Id-Type) and R (Range-Type). I agree that it might be better to use descriptive names for types and I think I will do so in the future!

  1. Why did you use all the time this operator overloading function. Could it not by also done with a simple function that can be called and returns the result?

That is a design choice by @juhasz. It could be done by a simple function, and it actually is nothing more that a function. You could actually call it by obj->operator()(arg_list). I have actually done that in include/rosa/agent/FunctionAbstractions.hpp:218.
For me, the choice to override the ()-operator makes sense here, as it is intuitive that a functionality is executable.

• juhasz raised a concern with this commit.May 14 2019, 8:24 AM

Having M_PI fixed as in my inline comment, MSVC complains about implicit conversions (line numbers in agent-functionalities.cpp are offset with 5 due to the fix for M_PI):

1>------ Build started: Project: agent-functionalities, Configuration: Debug x64 ------
1>agent-functionalities.cpp
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\utility(123): error C2220: warning treated as error - no 'object' file generated
1>C:\Users\juhas\Documents\GitHub\SoC_Rosa_repo\examples\agent-functionalities\agent-functionalities.cpp(121): note: see reference to function template instantiation 'std::pair<T,T>::pair<int,int,0>(_Other1 &&,_Other2 &&) noexcept' being compiled
1>        with
1>        [
1>            T=float,
1>            _Other1=int,
1>            _Other2=int
1>        ]
1>C:\Users\juhas\Documents\GitHub\SoC_Rosa_repo\examples\agent-functionalities\agent-functionalities.cpp(113): note: see reference to function template instantiation 'std::pair<T,T>::pair<int,int,0>(_Other1 &&,_Other2 &&) noexcept' being compiled
1>        with
1>        [
1>            T=float,
1>            _Other1=int,
1>            _Other2=int
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\utility(123): warning C4244: 'initializing': conversion from '_Ty' to '_Ty1', possible loss of data
1>        with
1>        [
1>            _Ty=int
1>        ]
1>        and
1>        [
1>            _Ty1=float
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\utility(123): warning C4244: 'initializing': conversion from '_Ty' to '_Ty2', possible loss of data
1>        with
1>        [
1>            _Ty=int
1>        ]
1>        and
1>        [
1>            _Ty2=float
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1547): warning C4244: 'argument': conversion from '_Ty' to 'D', possible loss of data
1>        with
1>        [
1>            _Ty=double
1>        ]
1>        and
1>        [
1>            D=float
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1601): note: see reference to function template instantiation 'std::_Ref_count_obj<_Ty>::_Ref_count_obj<double,double>(double &&,double &&)' being compiled
1>        with
1>        [
1>            _Ty=rosa::agent::LinearFunction<float,float>
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1602): note: see reference to function template instantiation 'std::_Ref_count_obj<_Ty>::_Ref_count_obj<double,double>(double &&,double &&)' being compiled
1>        with
1>        [
1>            _Ty=rosa::agent::LinearFunction<float,float>
1>        ]
1>C:\Users\juhas\Documents\GitHub\SoC_Rosa_repo\examples\agent-functionalities\agent-functionalities.cpp(116): note: see reference to function template instantiation 'std::shared_ptr<rosa::agent::LinearFunction<float,float>> std::make_shared<rosa::agent::LinearFunction<float,float>,double,double>(double &&,double &&)' being compiled
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1547): warning C4244: 'argument': conversion from '_Ty' to 'D', possible loss of data
1>        with
1>        [
1>            _Ty=int
1>        ]
1>        and
1>        [
1>            D=float
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1601): note: see reference to function template instantiation 'std::_Ref_count_obj<_Ty>::_Ref_count_obj<int,int>(int &&,int &&)' being compiled
1>        with
1>        [
1>            _Ty=rosa::agent::LinearFunction<float,float>
1>        ]
1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.20.27508\include\memory(1602): note: see reference to function template instantiation 'std::_Ref_count_obj<_Ty>::_Ref_count_obj<int,int>(int &&,int &&)' being compiled
1>        with
1>        [
1>            _Ty=rosa::agent::LinearFunction<float,float>
1>        ]
1>C:\Users\juhas\Documents\GitHub\SoC_Rosa_repo\examples\agent-functionalities\agent-functionalities.cpp(118): note: see reference to function template instantiation 'std::shared_ptr<rosa::agent::LinearFunction<float,float>> std::make_shared<rosa::agent::LinearFunction<float,float>,int,int>(int &&,int &&)' being compiled
1>Done building project "agent-functionalities.vcxproj" -- FAILED.
========== Build: 0 succeeded, 1 failed, 5 up-to-date, 0 skipped ==========

MSVC are very sensitive for types of literals (e.g., 0 is int, 0. is double, 0.f is float), so the following will work:

constexpr float Pi = (float) M_PI; // Explicit conversion to float.
RCL({
  {Categories::Bad, PartialFunction<float, float>({
    {{0.f, 3.f}, std::make_shared<LinearFunction<float, float>>
      (0.f, 1.f/3)},
    {{3.f, 6.f}, std::make_shared<LinearFunction<float, float>>
      (1.f, 0.f)},
    {{6.f, 9.f}, std::make_shared<LinearFunction<float, float>>
      (3.f, -1.f/3)},
  },0)},
  {Categories::Normal, PartialFunction<float, float>({
    {{6.f, 9.f}, std::make_shared<LinearFunction<float, float>>
      (-2.f, 1.f/3)},
    {{9.f, 12.f}, std::make_shared<LinearFunction<float, float>>
      (1.f, 0.f)},
    {{12.f, 15.f}, std::make_shared<LinearFunction<float, float>>
      (5.f, -1.f/3)},
  },0)},
  {Categories::Good, PartialFunction<float, float>({
    {{12.f, 15.f}, std::make_shared<LinearFunction<float, float>>
      (-4.f, 1.f/3)},
    {{15.f, 18.f}, std::make_shared<LinearFunction<float, float>>
      (1.f, 0.f)},
    {{18.f, 21.f}, std::make_shared<LinearFunction<float, float>>
      (7.f, -1.f/3)},
  },0)}
}),
RCS({
  {Categories::Bad, PartialFunction<float, float>({
    {{0.f, 3.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2, 0.5f)},
    {{3.f, 6.f}, std::make_shared<LinearFunction<float, float>>(1.f, 0.f)},
    {{6.f, 9.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2 + 3, 0.5f)},
  },0)},
  {Categories::Normal, PartialFunction<float, float>({
    {{6.f, 9.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2, 0.5f)},
    {{9.f, 12.f}, std::make_shared<LinearFunction<float, float>>(1.f, 0.f)},
    {{12.f, 15.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2 + 3, 0.5f)},
  },0)},
  {Categories::Good, PartialFunction<float, float>({
    {{12.f, 15.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2, 0.5f)},
    {{15.f, 18.f}, std::make_shared<LinearFunction<float, float>>(1.f, 0.f)},
    {{18.f, 21.f}, std::make_shared<SineFunction<float, float>>
      (Pi/3, 0.5f, -Pi/2 + 3, 0.5f)},
  },0)}
}, true)
/examples/agent-functionalities/agent-functionalities.cpp
123

M_PI does not compile with MSVC.

Please add the following as the very first(!) include directive at the beginning of the file:

#ifdef _WIN32
#define _USE_MATH_DEFINES
#include <cmath>
#endif
This commit now has outstanding concerns.May 14 2019, 8:24 AM
• juhasz added a comment.May 14 2019, 8:26 AM

@btutzer I can commit a fix if you agree with my suggested solution in R20:a94537b537bbdabf920b23f61c00f10e8b2d0388#8725. Let me know.

• btutzer added a comment.May 14 2019, 9:48 AM

Hi.
I think your fix makes sense.
As Pi is not needed by RoSA itself but only by apps and examples, the necessity to use the proposed include order is not a big restriction in my opinion.
Please push the fix.

Diffusion mentioned this in R20:7eccb1408351: Fixes R20:a94537b537bb.May 14 2019, 6:02 PM
• juhasz accepted this commit.May 14 2019, 6:07 PM

The problem reported in R20:a94537b537bb#8725 is fixed by R20:7eccb1408351.

All concerns with this commit have now been addressed.May 14 2019, 6:07 PM

Changes (5)

PathSize
examples/
agent-functionalities/
include/
rosa/
agent/
lib/
agent/
LinearFunctions.cpp
Functionality.cpp

R20:a94537b537bb

View Options

examples/agent-functionalities/agent-functionalities.cpp

Loading...
View Options

include/rosa/agent/Abstraction.hpp

Loading...
View Options

include/rosa/agent/LinearFunctions.hpp

Loading...
View Options

lib/agent/CMakeLists.txt

Loading...
View Options

lib/agent/LinearFunctions.cpp

Loading...