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

Remove ToString() entirely #487

Merged
merged 3 commits into from Dec 14, 2015

Conversation

Projects
None yet
1 participant
@rcurtin
Member

rcurtin commented Dec 1, 2015

I'll leave this pull request open for a week, for the sake of discussion, since it's a major change

In the past, the great idea was come upon that a ToString() method should be implemented in every class, then some glue logic could allow operator<<() for any arbitrary mlpack object. This was done with template metaprogramming (see sfinae_utility.hpp by Trironk, it is quite cool) and worked fine.

However, as time has gone on, it's unclear to me whether or not this feature is actually useful, and personally I am of the opinion that it should be removed, so I made a few commits removing all the ToString() functions and related functionality and made this pull request. Here are my arguments for why it isn't very useful:

  • It's not always clear how much should be printed in a ToString() call. Should we print all of a binary space tree? Only the first few levels? How much information does the user want? In many cases, the amount of information that could be printed is huge, so when a user sends an object to an ostream, they get reams and reams of output.
  • It's extra overhead for new contributors and also for maintenance, if their classes also have to have a ToString() method. And usually when these are written, it seems like simple copypasta and not much thought is given into precisely what to display (see the previous point).
  • It's not actually all that useful for debugging; often when debugging, I find myself ignoring the ToString() function and instead only printing the member of the object that I am interested in.

So, those are the primary reasons I think that we should remove---or at least rethink---the ToString() functionality. Like I said, I'll leave this open for a week for discussion, then merge in the change. So if you disagree with anything I've written here, or you find the ToString() functions particularly useful, feel free to comment! My perspective is not the only perspective. :)

rcurtin added some commits Dec 1, 2015

Remove ToString() from everything.
It's more hassle than it's worth; hard to maintain, difficult to know what
exactly should be printed, and maybe wasn't all that useful to begin with.
Removing this should reduce the API necessary for new contributors.

@rcurtin rcurtin merged commit a693c72 into mlpack:master Dec 14, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@rcurtin

This comment has been minimized.

Show comment
Hide comment
@rcurtin

rcurtin Dec 14, 2015

Member

Okay, nobody had any comments so I merged it in. :)

Member

rcurtin commented Dec 14, 2015

Okay, nobody had any comments so I merged it in. :)

@rcurtin rcurtin added this to the mlpack 1.1.0 milestone Dec 14, 2015

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