-
Notifications
You must be signed in to change notification settings - Fork 231
Added some unit tests for Astar and DP #57
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
Conversation
tofuwen
left a comment
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.
awesome work!
Only some very nits need to be addressed. :)
tests/TestAstar.py
Outdated
| def test_astar_simulate_linear_gaussian_with_local_score_BIC(self): | ||
| print('Now start test_astar_simulate_linear_gaussian_with_local_score_BIC ...') | ||
| truth_CPDAG_matrix = np.loadtxt("./TestData/test_exact_search_simulated_linear_gaussian_CPDAG.txt") | ||
| data = np.loadtxt("./TestData/test_exact_search_simulated_linear_gaussian_data.txt") |
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.
How do we generate this data? Do you mind adding the data generating script in this file as well (but comment out the code) --- this can ensure we know how the data is generated later.
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.
Thanks for the suggestion. I have added and commented the code to generate the data and ground truth.
|
In your screenshot, I saw 0,1,2,3 are printed. Do we need to print them? Maybe we can remove it? |
I notice that the numbers are printed when the function |
|
Yeah, please change the dag2cpdag. We don't the need the extra print in test, which is confusing. And in any industry level code, there shouldn't be print statement haha |
I notice that the reason is not the |
tofuwen
left a comment
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! Thanks for the great work!
cc @kunwuz to merge this
Updates
Test Plan
python -m unittest tests.TestAstar # should passpython -m unittest tests.TestDP # should pass