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

Feature/string attributes #15

Merged
merged 10 commits into from Dec 22, 2016
Merged

Feature/string attributes #15

merged 10 commits into from Dec 22, 2016

Conversation

kcning
Copy link
Contributor

@kcning kcning commented Dec 9, 2016

Hi, I added some new methods for #12!

    1) add support for string attribute
    2) add spec for string attribute
    1) add support for checking if a certain type of attribute exists
    2) add spec for the new methods
@paulgoetze
Copy link
Owner

Thanks, @kcning. Sorry for the late answer, for some reason I didn't receive any notice about this from Github. I will have a look within the next days.

Copy link
Owner

@paulgoetze paulgoetze left a comment

Choose a reason for hiding this comment

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

Awesome work all in all 👍

@@ -7,7 +7,7 @@ module Core
class Attribute
include Weka::Concerns::Persistent

TYPES = %(numeric nominal string date).freeze
TYPES = %i(numeric nominal string date)
Copy link
Owner

Choose a reason for hiding this comment

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

Nice catch! %(...) is actually just a string. It just worked, because include? also works on strings, but it should definitely be an array of symbols. 👍

Please .freeze the array again. TYPE should still not be changeable from outside.

@@ -63,6 +63,8 @@ def value_from(value, index)
value
elsif attribute.nominal?
attribute.value(value)
elsif attribute.string?
attribute.value(value)
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need an additional elsif here. The added lines could also be written as

# ...
elsif attribute.nominal? || attribute.string?

in line 64. Just to keep it DRY :)

alias with_attributes add_attributes
alias instances_count num_instances
alias attributes_count num_attributes
alias has_string_attribute? check_for_string_attributes
Copy link
Owner

Choose a reason for hiding this comment

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

2 unnecessary spaces between alias a ␣␣b.

type = map_attribute_type(type) unless type.is_a? Integer
return false if type.nil?
check_for_attribute_type(type)
end
Copy link
Owner

Choose a reason for hiding this comment

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

The return line should be the first in the method, so that you don't need to any unnecessary methods runs if type is nil.

For consistency within the project: is_a? should use parenthesis as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use map_attribute_type to convert valid attribute types to integers and undefined types to nil. So if we pass arbitrary string that's not valid, it becomes nil after the method call. At the moment calling map_attribute_type is necessary because check_for_attribute (a Java method) can only take Integer. Perhaps there're ways to work around this.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, right. Then we could just return -1 (never existing integer index for attribute types) instead of nil in map_attribute_type and the return false if type.nil? line can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks!


def map_attribute_type(type)
type = type.downcase.to_sym
return nil unless Attribute::TYPES.include? type
Copy link
Owner

Choose a reason for hiding this comment

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

You don't need to return nil explicitly. return unless works just fine. Please also use parenthesis with include? in order to keep the style of method calls consistent within the project.

context 'dataset has string attribute' do
subject { load_instances('weather.string.arff') }

it 'returns true if string attribute exists' do
Copy link
Owner

Choose a reason for hiding this comment

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

The if part of the description can be pulled, it's already given by the surrounding context.

@@ -545,4 +553,100 @@
end
end
end

describe '#has_string_attribute?' do
it 'returns false if no string attribute exists' do
Copy link
Owner

Choose a reason for hiding this comment

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

Should the if part in the description also be written as context 'if...'?

expect(subject.has_string_attribute?).to be false
end

context 'dataset has string attribute' do
Copy link
Owner

Choose a reason for hiding this comment

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

I usually try to write context + it descriptions, so that you can read them as sentence if they are combined.
Would it make sense to adjust the context description to sth. like 'if dataset has string attribute'?

end

context 'checking with String argument' do
%w(string numeric nominal date).each do |type|
Copy link
Owner

Choose a reason for hiding this comment

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

Should it also use Weka::Core::Attribute::TYPES.each as below?
(Or then rather Weka::Core::Attribute::TYPES.map(&:to_s).each...)

overcast,'Partly cloudy skies in the morning will give way to cloudy skies during the afternoon. High 46F. Winds NW at 25 to 30 mph.',75,70,TRUE,yes
overcast,'Partly cloudy. Low 27F. Winds NW at 5 to 10 mph.',72,90,TRUE,yes
overcast,'Considerable clouds early. Some decrease in clouds late. Low 34F. Winds light and variable',81,75,FALSE,yes
rainy,'Overcast with rain showers at times. Low 36F. Winds light and variable. Chance of rain 60%.',71,91,TRUE,no
Copy link
Owner

Choose a reason for hiding this comment

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

Don't know whether this also applies for this file, but there is a missing empty line at the end of the file.

@kcning
Copy link
Contributor Author

kcning commented Dec 20, 2016

Oke, I changed a bit. And thanks for the review :)

@paulgoetze paulgoetze merged commit d8202b6 into paulgoetze:feature/string-attributes Dec 22, 2016
@paulgoetze
Copy link
Owner

paulgoetze commented Dec 22, 2016

Thanks, @kcning! I will publish a new version & gem release later today, including all the string attribute changes.

@kcning kcning deleted the feature/string-attributes branch December 23, 2016 02:48
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

2 participants