Closed xuan-w closed 3 years ago
Besides, I am really confused that why my pull request would contain 4 "merging commits". I thought it should be fast forward merge and no merging commits should exist. Never see this on my own git server.
Hi Xuan, thanks for the pull-request. Your code looks really well documented, thanks! Please allow me some time to test it. I like the idea of keeping the conversion outside the BN class for simplicity, and perhaps we could called it if the format is type "Cell Collective". Even Thomas' code could possibly be outside. Let's give it some though. =]
Talking about unit test, I am a little unsure about how the test for file format conversion should work. Assert that converted file content to be exactly same as expected? Or just test a few lines of the converted file? Let me think more about it. Ideally a unit test should cover all strange cases like strange node names. Then BN should be able to read the converted file and do some computation. We can assert the bias, k_eff or other measurements to be within expectation? That should be a better way for the test. In that case, if BN was changed so that it can no longer read the converted files, we will know immediately.
@xuan-w, I have resolved the conflicts on this pull request. Please let me know if you want me to push it. Are you done with the test cases?
@xuan-w, I'm closing this PR since this now conflicts with v0.1. Let's make a new PR to include the CellCollective I/O methods as we discussed. Cheers bud!
Sorry sent this late. I took a little time to rewrite part of it to make it compatible to python 2.5 2.6 2.7 3.5 3.6 (other version not tested). Now it is 100x faster than my older version and is free from many possible issues due to strange node names (such as node names containing "/" "\" "-" "NOT" "(" ")"). I wondered why I wrote a Boolean interpreter at the beginning. Now I am using python eval() (Just like Thomas did, though he is dealing with a slightly different file format). It is not surprising that this is much faster.
Add some comments to help understand how those code work.
I saw Thomas's pull request. Maybe I should modify it a little to put it inside BN.from_string() too. However, like we talked before, some networks' LUT are too huge. Thus use from_string() -> to_cnet() to convert file format would consume too much memory.
Looking forward to hear your opinion