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.

Sign up for GitHub

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

Jump to bottom

Read only property #4549

Merged
merged 3 commits into from Mar 12, 2019
Merged

Read only property #4549

merged 3 commits into from Mar 12, 2019

Conversation

gf712
Copy link
Member

@gf712 gf712 commented Mar 1, 2019

@karlnapf Is this what you had in mind?

@gf712
Copy link
Member Author

gf712 commented Mar 1, 2019

We can't call the enum field CONST because msvc thinks we are using the keyword const...

@gf712
Copy link
Member Author

gf712 commented Mar 1, 2019

Should a constant parameter be put-able once for initialisation, or would it always be read-only?

lisitsyn
lisitsyn reviewed Mar 1, 2019
View reviewed changes
src/shogun/base/AnyParameter.h Outdated
@@ -37,7 +37,8 @@ namespace shogun
HYPER = 1u << 0,
GRADIENT = 1u << 1,
MODEL = 1u << 2,
AUTO = 1u << 10
AUTO = 1u << 10,
CONST = 1u << 11
Copy link
Member

Choose a reason for hiding this comment

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

READ_ONLY might be a better name to avoid clashes

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, I didn't realise that it could clash with C++ keywords...

karlnapf
karlnapf reviewed Mar 2, 2019
View reviewed changes
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.

Yes!
This error should pop up in a few meta examples now

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2019

I think always read only, never put able. And then we try to use factories.... let’s see if that approach works at least. Can you mark feature matrix of dense feats also read only?

@gf712
Copy link
Member Author

gf712 commented Mar 2, 2019

So should the kernel ,for example, only be initialised with init(lhs,rhs), instead of sg.kernel("name", lhs=lhs, rhs=rhs)?

@karlnapf
Copy link
Member

karlnapf commented Mar 2, 2019

Exactly! That’s the the point

karlnapf
karlnapf reviewed Mar 3, 2019
View reviewed changes
examples/meta/src/gaussian_process/regression.sg
@@ -11,7 +11,8 @@ Labels labels_test = labels(f_labels_test)
#![create_features]

#![create_appropriate_kernel_and_mean_function]
Kernel k = kernel("GaussianKernel", lhs=features_train, rhs=features_train, log_width=0.0)
Kernel k = kernel("GaussianKernel", log_width=0.0)
Copy link
Member

Choose a reason for hiding this comment

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

yes!

karlnapf
karlnapf reviewed Mar 3, 2019
View reviewed changes
src/shogun/kernel/Kernel.cpp
@@ -922,7 +922,8 @@ void CKernel::register_params() {
"Cache size in MB.");

SG_ADD(
&lhs, "lhs", "Feature vectors to occur on left hand side.");
&lhs, "lhs", "Feature vectors to occur on left hand side.",
ParameterProperties::READONLY);
Copy link
Member

Choose a reason for hiding this comment

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

rhs should also be read-only

karlnapf
karlnapf reviewed Mar 3, 2019
View reviewed changes
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.

add rhs and then we can merge this.

@vigsterkr
Copy link
Member

vigsterkr commented Mar 4, 2019 •
edited

@karlnapf i'm just wondering in case of DenseFeatures the feature_matrix could be defined as a const and that should have the same effect... or?

@karlnapf
Copy link
Member

karlnapf commented Mar 4, 2019

Unfortunately no. It was my preferred solution as well but the any framework makes the compiler freak out if we do that (e.g clone, put) and then when I talked to @lisitsyn and he had some other reasons why not to do that. This pr here is the result of our discussion

@lisitsyn
Copy link
Member

lisitsyn commented Mar 4, 2019

@vigsterkr I am just afraid adding constness would make things more complex. It could make sense in pure C++ but since we pass the runtime boundary in the interfaces it might be not that good.

@lisitsyn
Copy link
Member

lisitsyn commented Mar 4, 2019

Ah and this also nearly doubles the amount of generated template code.

@lisitsyn lisitsyn closed this Mar 4, 2019
@lisitsyn lisitsyn reopened this Mar 4, 2019
karlnapf
karlnapf reviewed Mar 6, 2019
View reviewed changes
src/shogun/kernel/Kernel.cpp
SG_ADD(
&rhs, "rhs", "Feature vectors to occur on right hand side.",
ParameterProperties::READONLY);
SG_ADD(&lhs_equals_rhs, "lhs_equals_rhs",
Copy link
Member

Choose a reason for hiding this comment

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

also readonly!

karlnapf
karlnapf approved these changes Mar 6, 2019
View reviewed changes
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.

Fine to merge from my side!

@karlnapf
Copy link
Member

karlnapf commented Mar 6, 2019

CI problems?

@karlnapf karlnapf merged commit 463b648 into shogun-toolbox:develop Mar 12, 2019
ktiefe pushed a commit to ktiefe/shogun that referenced this pull request Jul 30, 2019
@ktiefe
Read only property (shogun-toolbox#4549)
db9beee 
* example implementation
* replaced put with init in kernel
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lisitsyn lisitsyn lisitsyn left review comments

@karlnapf karlnapf karlnapf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gf712 @karlnapf @vigsterkr @lisitsyn