SciRuby / daru

Data Analysis in RUby
BSD 2-Clause "Simplified" License
1.03k stars 139 forks source link

#308 test for block handling in from_csv #413

Closed parthm closed 6 years ago

parthm commented 6 years ago

Issue #308. Adds a test for block handling for from_csv.

parthm commented 6 years ago

Ah. My bad. Needs some more work as per comments on #326. This is more of a test case for header processing which is a good idea to have anyway I suppose. Will update this further.

parthm commented 6 years ago

Hmm. Is the block functionality removed? or am I missing something. Can't seem to get be block to be called.

dataframe.rb has the following:

  47       def from_csv path, opts={}, &block
  48         Daru::IO.from_csv path, opts, &block
  49       end

However, daru/io/io.rb doesn't seem to be handling block.

 78       def from_csv path, opts={}
 79         daru_options, opts = from_csv_prepare_opts opts
 80         # Preprocess headers for detecting and correcting repetition in
 81         # case the :headers option is not specified.
 82         hsh =
 83           if opts[:headers]
 84             from_csv_hash_with_headers(path, opts)
 85           else
 86             from_csv_hash(path, opts)
 87               .tap { |hash| daru_options[:order] = hash.keys }
 88           end
 89         Daru::DataFrame.new(hsh,daru_options)
 90       end
parthm commented 6 years ago

Phew. I think this might be OK. Please let me know in case there are any suggestion. Sorry for the noise, was a little confused initially :grin:

athityakumar commented 6 years ago

@zverok @v0dro - I'm not sure of the workflow we need to follow for io / view changes during the porting phase. Should such changes be directly sent to daru-io / daru-view, or should merged PRs here be replicated again in daru-io / daru-view and then removed here during porting phase?

zverok commented 6 years ago

For me, it looks like block supporting functionality is unnecessary. I'd vote for dropping it (in daru-io -- transition to it would introduce a lost of incompatibilities anyways)

parthm commented 6 years ago

Thinking about this a bit more, I am in agreement with @zverok . Is there a strong use case for this feature? Based on the test cases I implemented in this patch, the same actions could as easily be done by creating the DataFrame and updating the data following this. We could discard this PR and create a separate issue to clean up the docs and do any cleanup related to removal of this feature.

parthm commented 6 years ago

@zverok @v0dro Since there are no objects to removing the block support as discussed above. We can probably close this. #428 has been filed to track removal of block handling.