Skip to content

Conversation

robieta
Copy link
Contributor

@robieta robieta commented Mar 28, 2018

There is already static test data for wide-deep, so I simply used that rather than adding synthetic data.

@robieta
Copy link
Contributor Author

robieta commented Mar 29, 2018

@karmel Ready for review.

Copy link
Contributor

@karmel karmel left a comment

Choose a reason for hiding this comment

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

A few comments, nothing major.


# Used for end-to-end tests.
shutil.copyfile(self.input_csv, os.path.join(self.temp_dir, 'adult.data'))
shutil.copyfile(self.input_csv, os.path.join(self.temp_dir, 'adult.test'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems unusual to write one file and then copy it twice; why not just create the two CSVs above with the desired names, rather than reaching out to shutil?

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 realized I wasn't even using the full test csv. Also, changed to just write rather than shutil.

def eval_input_fn():
return input_fn(test_file, 1, False, flags.batch_size)

loss_prefix = {'wide': 'linear/', 'deep': 'dnn/'}.get(flags.model_type, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make the name dict a module-var, for greater visibility?

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.

@robieta robieta merged commit 9cc7eac into master Mar 29, 2018
@robieta robieta deleted the extend_tests branch March 29, 2018 20:15
omegafragger pushed a commit to omegafragger/models that referenced this pull request May 15, 2018
…rations. (tensorflow#3798)

* add end-to-end tests for wide_deep

delint

* address PR comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants