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

Make SG_ADD register parameters into the new parameter map #4048

Merged
merged 1 commit into from Dec 25, 2017

Conversation

lisitsyn
Copy link
Member

No description provided.

@@ -88,12 +88,14 @@ class GradientModelSelectionCostFunction: public FirstOrderCostFunction
SG_ADD((CSGObject **)&m_obj, "GradientModelSelectionCostFunction__m_obj",
"obj in GradientModelSelectionCostFunction", MS_NOT_AVAILABLE);
m_func_data = NULL;
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlnapf need your comments here. They were registered by value which is not good.

@@ -216,6 +216,13 @@ template <class T> class SGSparseVector : public SGReferencedData

};

template <class T>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything that goes into Any should have operator==, that's why I added it here

@lisitsyn lisitsyn force-pushed the sg-add-map branch 2 times, most recently from c15b034 to c29d8e5 Compare December 24, 2017 19:05
@lisitsyn
Copy link
Member Author

lisitsyn commented Dec 24, 2017

Apparently this slows down the compilation as running tests on travis times out most of the times.

@codecov
Copy link

codecov bot commented Dec 24, 2017

Codecov Report

❗ No coverage uploaded for pull request base (develop@c7af75e). Click here to learn what that means.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             develop    #4048   +/-   ##
==========================================
  Coverage           ?   56.55%           
==========================================
  Files              ?     1329           
  Lines              ?    92734           
  Branches           ?        0           
==========================================
  Hits               ?    52444           
  Misses             ?    40290           
  Partials           ?        0
Impacted Files Coverage Δ
src/shogun/lib/SGSparseVector.h 16.66% <0%> (ø)
...c/shogun/modelselection/GradientModelSelection.cpp 86.2% <100%> (ø)
src/shogun/base/SGObject.h 100% <100%> (ø)

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 c7af75e...3dde105. Read the comment docs.

@vigsterkr
Copy link
Member

@lisitsyn the reason for the compilation slowdown is because you've changed a .h that basically being used everywhere hence the ccache-ed objects cannot be used... hence travis needs to compile everything from scratch... see in case of appveyor the compilation time is always 40+ mins ;)

@vigsterkr vigsterkr merged commit 060618a into shogun-toolbox:develop Dec 25, 2017
@karlnapf
Copy link
Member

ho ho ho

#define SG_ADD4(param, name, description, ms_available) \
{ \
m_parameters->add(param, name, description); \
watch_param(name, param); \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this reminds me. We currently have two maps: one for registering all paramters, one for modelselection available only.
I suggest we don't keep this design with tags, but rather have one and somehow mark them otherwise ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the parameter we store should be 'rich'. This means it is not just 'Any' but 'Any' + extras. Such as this model_selection_enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep I agree. I think this is a place to include the domain bounds, information about gradients, sampling callbacks (?), priors, etc

Copy link
Member

@karlnapf karlnapf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎄

@lisitsyn lisitsyn deleted the sg-add-map branch December 30, 2017 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants