Skip to content

Conversation

@mattiaciollaro
Copy link
Contributor

The current header argument in the np.savetxt call inside of the points_to_csv method is not compatible with the format expected from the initialize_df method.

A call to initialize_df when the argument is a pandas DataFrame constructed by reading the output of points_to_csv with pandas.read_csv will fail because of a wrong format of the header.

In particular, the issues here are the following:

  1. the header of the csv file is generated with a leading pound character ('#') that comes from the default comments='#' of np.savetxt

  2. the column names are prefixed with an unnecessary space in by ', '.join(self.space.keys + ['target']) inside of points_to_csv.

This pull request fixes the above two issues. I tested these changes on a notebook that I am happy to share upon request.

At the cost of making pandas an additional dependency, it may be easier to use pandas in methods such as points_to_csv and initialize_df.

@codecov-io
Copy link

Codecov Report

Merging #80 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #80   +/-   ##
=======================================
  Coverage   82.75%   82.75%           
=======================================
  Files           4        4           
  Lines         290      290           
  Branches       34       34           
=======================================
  Hits          240      240           
  Misses         45       45           
  Partials        5        5
Impacted Files Coverage Δ
bayes_opt/bayesian_optimization.py 66.08% <0%> (ø) ⬆️

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 44bd13c...ddf762d. Read the comment docs.

@fmfn
Copy link
Member

fmfn commented Jan 4, 2018

Thanks for the PR. Indeed, pandas is not used in points_to_csv as to avoid the unnecessary dependence, and I would like to keep it this way.

@fmfn fmfn merged commit bd5d4fe into bayesian-optimization:master Jan 4, 2018
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.

3 participants