Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ParametersObservers] Main refactor of the parameter's observers feature. #3939

Merged
merged 3 commits into from Aug 5, 2017

Conversation

Projects
None yet
2 participants
@geektoni
Copy link
Contributor

geektoni commented Jul 20, 2017

  • Move all parameter observers into separate directory;
  • Add ObservedValue class which will store a measurement;
  • Refactor SGObject methods name (observe());
  • Add SG_OBS_VALUE_TYPE to indicate what is the observed value for;
  • Add macro to check the observed value type;
  • Update register_observable_param by using SG_OBS_VALUE_TYPE as type;
  • Move filter() implementation to ParameterObserverInterface.cpp;
  • Refactor unit tests to use the new implementation;
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 20, 2017

Codecov Report

Merging #3939 into feature/parameter_observers_refactor will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@                           Coverage Diff                            @@
##           feature/parameter_observers_refactor    #3939      +/-   ##
========================================================================
+ Coverage                                 55.79%   55.79%   +<.01%     
========================================================================
  Files                                      1356     1356              
  Lines                                     94000    94009       +9     
========================================================================
+ Hits                                      52445    52451       +6     
- Misses                                    41555    41558       +3
Impacted Files Coverage Δ
src/shogun/io/TBOutputFormat.h 0% <ø> (ø) ⬆️
...rameter_observers/ParameterObserverTensorBoard.cpp 75% <ø> (ø)
...ib/parameter_observers/ParameterObserverScalar.cpp 38.09% <0%> (ø)
...parameter_observers/ParameterObserverHistogram.cpp 0% <0%> (ø)
src/shogun/base/SGObject.cpp 68.44% <0%> (-1.02%) ⬇️
...parameter_observers/ParameterObserverInterface.cpp 82.35% <100%> (ø)
src/shogun/lib/parameter_observers/ObservedValue.h 69.23% <69.23%> (ø)
src/shogun/io/TBOutputFormat.cpp 84.74% <75%> (-1.22%) ⬇️
src/shogun/lib/DataType.cpp 67.13% <0%> (-0.36%) ⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6c6db...2bd4387. Read the comment docs.

@@ -36,7 +37,7 @@
namespace shogun
{
typedef std::map<BaseTag, Any> ParametersMap;
typedef std::map<std::string, std::pair<std::string, std::string>>
typedef std::map<std::string, std::pair<SG_OBS_VALUE_TYPE, std::string>>

This comment has been minimized.

@vigsterkr

vigsterkr Jul 28, 2017

Member

does it need to be a sorted map? :)

const std::string& description)
{
m_list_obs_params[name] = std::make_pair(type, description);
}

std::string type_name(SG_OBS_VALUE_TYPE type)

This comment has been minimized.

@vigsterkr

vigsterkr Jul 28, 2017

Member

this is for debug purposes only?
or is there somewhere we use type_name internally?

This comment has been minimized.

@geektoni
* @param type the type of this observed parameters
*/
ObservedValue(Any value, SG_OBS_VALUE_TYPE type)
: m_step(0), m_name(""), m_value(value), m_type(type)

This comment has been minimized.

@vigsterkr

vigsterkr Jul 28, 2017

Member

that empty string as m_name feels a bit 🍶 🐑

* @return an ObservedValue object initialized
*/
SG_FORCED_INLINE ObservedValue
make_observation(int64_t step, std::string& name, Any value)

This comment has been minimized.

@vigsterkr

vigsterkr Jul 28, 2017

Member

this could be a static function of ObservedValue, just like a factory function and returns a std::shared_ptr/some/*

* @param value time point we want to convert
* @return the time point converted to std::time_t
*/
inline double convert_to_millis(const time_point& value)

This comment has been minimized.

@vigsterkr

vigsterkr Jul 28, 2017

Member

SG_FORCED_INLINE

geektoni added some commits Jul 19, 2017

[ParametersObservers] Main refactor of the parameter's observers feat…
…ure.

* Move all parameter observers into separate directory;
* Add ObservedValue class which will store a measurement;
* Refactor SGObject methods name (observe());
* Add SG_OBS_VALUE_TYPE to indicate what is the observed value for;
* Add macro to check the observed value type;
* Update register_observable_param by using SG_OBS_VALUE_TYPE as type;
* Move filter() implementation to ParameterObserverInterface.cpp;
* Refactor unit tests to use the new implementation;

@geektoni geektoni force-pushed the geektoni:parameter_observers_refactor branch from d304076 to 2bd4387 Jul 28, 2017

@vigsterkr vigsterkr merged commit 34aaee5 into shogun-toolbox:feature/parameter_observers_refactor Aug 5, 2017

3 of 4 checks passed

codecov/patch 50% of diff hit (target 55.79%)
Details
codecov/project 55.79% (+<.01%) compared to 5e6c6db
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.