-
Notifications
You must be signed in to change notification settings - Fork 23
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
Refactoring Matrix Generation Library in Slate #145
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some stylistic changes, but overall looks right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more feedback. I didn't look at everything. A lot of these comments are about how the include files are structured and included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! A few minor things to cleanup or think about. I'll fix these.
…d a small code snippet to generate_matrix_tz.cc.
…s configure_seed, decode_matrix, etc.
…ges in PR comments.
…late.hh, and moved genearte_matrix.hh to include directory.
…dating test routines to include matgen.hh.
…random.cc and random.hh to matgen directory, and cleanup of files.
…ving testsweeper.hh into matgen directory such that genearte_matrix_utils.cc does not rely on testsweeper.
… lambda function.
… ANSI color codes into file.
…void nan comparison.
4a14a81
to
35f9053
Compare
If someone wants to look over my changes, that would be great. Otherwise I'll merge it in sometime this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your changes look good to me. The only issue that I still see in the overall PR is some unnecessary #include
's. For example, chrono
is included in most of the matgen CC files, but only used in matgen/generate_matrix_utils.cc. But, that's not a huge issue.
Refactor current SLATE matrix generation into its own library.