Adds support to missing data for where clause #302

athityakumar · 2017-02-06T11:10:13Z

Fixes issue #246. Sample output for the building pass is shown below.

2.3.1 :002 > v= Daru::Vector.new([1,2,3,Float::NAN, nil])
 => #<Daru::Vector(5)>
   0   1
   1   2
   2   3
   3 NaN
   4 nil 
2.3.1 :003 > v.where(v.lt(4))

 => #<Daru::Vector(3)>
   0   1
   1   2
   2   3 
2.3.1 :004 > v.where(v.lt(2))

 => #<Daru::Vector(1)>
   0   1 
2.3.1 :005 > v.where(v.lt(3))

 => #<Daru::Vector(2)>
   0   1
   1   2 
2.3.1 :006 > v.where(v.gt(1))

 => #<Daru::Vector(2)>
   1   2
   2   3

Ping @v0dro @zverok @lokeshh - Please review this PR when you're free. 😄

Changed the C offsenses of travis CI Fixes Travis CI error : Favor unless over if

zverok

Tests, please?

(And, side note: I'd advise you not to rush fixing each and every issue in a single day. Better to work on something relatively large, to understand our code quality expectations and processes. We greatly appreciate your effort, though!)

athityakumar · 2017-02-06T18:50:41Z

Tests, as in using Travis CI or test-unit gem?

zverok · 2017-02-06T18:54:04Z

@athityakumar I've meant specs -- there are ton of them & you can run them locally by just rspec or rspec path/to/your/spec.rb

v0dro · 2017-02-07T10:29:47Z

@athityakumar I suggest you first go through the CONTRIBUTING file and understand our processes properly before submitting PRs. We're all volunteers and it saves our time if you abide by some formal processes.

v0dro · 2017-02-07T10:30:27Z

Also, submitting a PR without tests is not acceptable. Especially if you want to apply for GSOC.

v0dro · 2017-02-07T10:31:41Z

lib/daru/core/query.rb

@@ -39,7 +39,7 @@ def inspect

      class << self
        def apply_scalar_operator operator, data, other
-          BoolArray.new data.map { |d| !!d.send(operator, other) }
+          BoolArray.new data.map { |d| !!d.send(operator, other) unless d.nil? }


Missing data is not just nil. What if the Vector contains strings combined with a Float::NaN? This will fail in that case.

See this: https://github.com/SciRuby/daru/blob/master/lib/daru.rb#L33

@v0dro Float::NaN I think are only used in numerical domain. I don't think they ever occur with strings or any other object except numbers.

athityakumar · 2017-02-07T12:07:38Z

@v0dro : Thanks for mentioning about the CONTRIBUTING.md file - I just read through it. I'll add the tests in a day, after including the other missing data types and let you know for reviewing the PR again.

athityakumar · 2017-02-07T21:04:52Z

@v0dro @zverok : The PR is ready for another review.

Updates -

(1) Thanks for the rspec and rubocop references. They really made testing the changes simpler.
(2) Support added for other missing data types like Float:NAN and Float:NAN in string.
(3) Tests have been written for Daru::Vector#where and Daru::DataFrame#where.

Any changes required?

zverok · 2017-02-08T10:48:25Z

spec/core/query_spec.rb

-          names: ['sameer', 'john', 'james', 'omisha', 'priyanka', 'shravan']
+          number: [1,2,3,4,5,6,nil],
+          sym: [:one, :two, :three, :four, :five, :six, :seven],
+          names: ['sameer', 'john', 'james', 'omisha', 'priyanka', 'shravan', Float::NAN.to_s]


Why Float::NAN.to_s should be considered as a missing value?

That was meant to be Float::NAN missing type as mentioned here. I will change that in the next commit, along with other changes. 😄

zverok · 2017-02-08T10:51:24Z

spec/core/query_spec.rb

+          names: [Float::NAN.to_s]
+          }, index: Daru::Index.new([6])
+        )
+        expect(@df.where(@df[:sym].eq(:seven))).to eq(answer)


There is an old saying "Don't believe test you've never seen failing". It means: you should first make sure this test is failing without your patch, then apply the patch, then check test is green.
Hint: it doesn't check anything (on current daru master it will work).

Ah, my bad. Will it be fine if I include the test case provided by @v0dro at issue #246 ?

Yep, think so.

athityakumar · 2017-02-08T14:39:02Z

@zverok : I've added the specific test that would have failed on the current master branch version of daru. As suggested, I've also added my test variables in letformat - however, it does seem a bit different / inconsistent with the rest of the tests in the spec/core/query_spec.rb file, as they don't use let format.

zverok · 2017-02-09T08:13:18Z

however, it does seem a bit different / inconsistent with the rest of the tests in the spec/core/query_spec.rb file, as they don't use let format.

Yes, part of the specs are just old and written without much attention. We want to have them rewritten in modern style with time, yet it was not done yet.

zverok · 2017-02-09T08:14:57Z

spec/core/query_spec.rb

+      let(:dv1) { Daru::Vector.new([1,11,32,Float::NAN,nil]) }
+      let(:dv2) { Daru::Vector.new([1,11]) }
+      it "handles empty data" do
+        expect(dv1.where(dv1.lt(14))).to eq(dv2)


I am not sure you need to create separate dv, expect(dv1.where(dv1.lt(14))).to eq(Daru::Vector.new([1,11])) should be enough.

athityakumar · 2017-02-09T11:54:03Z

@v0dro @zverok : Made the required changes in tests. 😄

athityakumar mentioned this pull request Feb 6, 2017

where clause should support missing data. #246

Closed

athityakumar changed the title ~~Adds support to missing data for where clause - nil? check~~ Adds support to missing data for where clause Feb 6, 2017

Adds support to missing data for where clause - nil? check

dad9925

Changed the C offsenses of travis CI Fixes Travis CI error : Favor unless over if

athityakumar force-pushed the where-module-exception branch from f8d00eb to dad9925 Compare February 6, 2017 16:26

zverok requested changes Feb 6, 2017

View reviewed changes

v0dro requested changes Feb 7, 2017

View reviewed changes

Added check for Float:NaN in where clause exception

27ab6e0

athityakumar force-pushed the where-module-exception branch from 13e8865 to 54462e3 Compare February 7, 2017 19:15

Adds tests for Daru::DataFrame#where and Daru::Vector#where

b114c5b

athityakumar force-pushed the where-module-exception branch from 54462e3 to b114c5b Compare February 7, 2017 21:04

zverok requested changes Feb 8, 2017

View reviewed changes

Adds specific test for where clause empty data

2285b54

zverok reviewed Feb 9, 2017

View reviewed changes

zverok approved these changes Feb 9, 2017

View reviewed changes

athityakumar force-pushed the where-module-exception branch from a53d3f5 to a52aabe Compare February 9, 2017 10:54

Removes let initialisation of expected output variable

8aa5414

athityakumar force-pushed the where-module-exception branch from a52aabe to 8aa5414 Compare February 9, 2017 11:00

v0dro approved these changes Feb 9, 2017

View reviewed changes

v0dro merged commit 8a4bb1c into SciRuby:master Feb 9, 2017

athityakumar mentioned this pull request Feb 13, 2017

Fixes build error #306

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds support to missing data for where clause #302

Adds support to missing data for where clause #302

athityakumar commented Feb 6, 2017 •

edited

Loading

zverok left a comment

athityakumar commented Feb 6, 2017

zverok commented Feb 6, 2017

v0dro commented Feb 7, 2017

v0dro commented Feb 7, 2017

v0dro Feb 7, 2017

v0dro Feb 7, 2017

lokeshh Feb 13, 2017

athityakumar commented Feb 7, 2017 •

edited

Loading

athityakumar commented Feb 7, 2017

zverok Feb 8, 2017

athityakumar Feb 8, 2017

zverok Feb 8, 2017

athityakumar Feb 8, 2017

zverok Feb 8, 2017

athityakumar commented Feb 8, 2017 •

edited

Loading

zverok commented Feb 9, 2017

zverok Feb 9, 2017

athityakumar commented Feb 9, 2017 •

edited

Loading

Adds support to missing data for where clause #302

Adds support to missing data for where clause #302

Conversation

athityakumar commented Feb 6, 2017 • edited Loading

zverok left a comment

Choose a reason for hiding this comment

athityakumar commented Feb 6, 2017

zverok commented Feb 6, 2017

v0dro commented Feb 7, 2017

v0dro commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athityakumar commented Feb 7, 2017 • edited Loading

athityakumar commented Feb 7, 2017

Updates -

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

athityakumar commented Feb 8, 2017 • edited Loading

zverok commented Feb 9, 2017

Choose a reason for hiding this comment

athityakumar commented Feb 9, 2017 • edited Loading

athityakumar commented Feb 6, 2017 •

edited

Loading

athityakumar commented Feb 7, 2017 •

edited

Loading

athityakumar commented Feb 8, 2017 •

edited

Loading

athityakumar commented Feb 9, 2017 •

edited

Loading