Benchmarking #2185
Benchmarking #2185
Conversation
| filepath_send | ||
| ); | ||
| if out { | ||
| eprintln!( |
There was a problem hiding this comment.
Use log::info! or log::warn! instead
| fn hex_to_binary(hex_string: &str, filepath_send: &mut File) -> io::Result<()> { | ||
| fn hex_to_binary( | ||
| hex_string: &str, | ||
| filepath_send: &mut Option<File>, |
There was a problem hiding this comment.
Seems like the &mut Option<File> pattern is used to indicate using either STDOUT or a specific file. It might be better to wrap this using something like OutputFile.
| @@ -0,0 +1,93 @@ | |||
| suite: | |||
| cases: | |||
| - command: cargo run -- --from /Users/Angelica/Desktop/calyx/tools/data-conversion/testsuite/inputs/test_1.in | |||
There was a problem hiding this comment.
Should not commit files with hardcoded file paths.
There was a problem hiding this comment.
Looking cool! Here are a couple of low-level code suggestions.
And one main organizational suggestion: if I'm understanding correctly, the binary_to_fixed_tests.py script is the "test oracle" that generates the expected outputs that we then compare against. Is that right? If so, I think we should probably not commit these *.expect files to git.
Here's my reasoning: in most situations where we do Runt-style snapshot testing, there is no test oracle. There is only the program under test (PUT) itself. All we're doing is "locking in" a specific set of outputs from the PUT by checking them into git. But here, you have an oracle that generates these—so in a way, having both the Python script to generate the *.expect outputs and the files themselves is redundant. We could clean up this redundancy if we were to just say that the testing procedure has two steps: (1) run binary_to_fixed_tests.py to generate the expected outputs, and then (2) run runt to check our new data conversion tool.
Does that make sense?
data-conversion/src/main.rs
Outdated
There was a problem hiding this comment.
Looks like this re-added the old file not under tools/. Could you please git rm this?
| input_dir = "/Users/Angelica/Desktop/calyx/tools/data-conversion/testsuite/inputs" | ||
| expect_dir = "/Users/Angelica/Desktop/calyx/tools/data-conversion/testsuite/expect" | ||
| yaml_filename = "/Users/Angelica/Desktop/calyx/tools/data-conversion/testsuite/testsuite.yaml" |
There was a problem hiding this comment.
So that this code will run on other people's machines too, let's use relative paths (e.g., ../testsuite/inputs).
tools/data-conversion/src/main.rs
Outdated
| /// file to convery to | ||
| #[argh(option)] | ||
| /// optional file to convery to | ||
| #[argh(option, default = "String::from(\"\")")] | ||
| to: String, |
There was a problem hiding this comment.
If you make this an Option<String>, then the default (no output file; print to stdout) will be easier to distinguish:
https://doc.rust-lang.org/std/option/
This way, usage will look like this:
- To print to a file, use
--to something.txt. - To print to stdout, use nothing at all.
(So there is no need for a separate -o option.)
There was a problem hiding this comment.
I think this YAML file is no longer used, right? If so, let's remove it.
There was a problem hiding this comment.
This file also may be unused?
There was a problem hiding this comment.
I think this file may be unused? If so, let's delete it.
| tests = generate_tests(num_tests) | ||
| results = [] | ||
| for i in range(len(tests)): | ||
| results.append(convert_binary_to_fixed(tests[i], -4)) |
There was a problem hiding this comment.
You might consider testing a range of exponents instead of just one.
| } | ||
|
|
||
| // Convert result to a fixed-point decimal representation | ||
| let fixed_value = result as f32; |
There was a problem hiding this comment.
This implementation of binary_to_fixed_bit_slice looks to have been changed from the one in #2176, but I'm not sure I understand this new implementation. It looks to me like this is always going to print an integer? (Also, maybe we should move this change to #2176 to keep the discussion in one place.)
…n instead of using runt
…t the values of the output and the expected value instead of the string represenation of it.
…kes one file with many test cases instead of multiples files, each with one test case.
This pull request introduces a test suite using runt meant to compare the implementations of the two different implementations of binary to fixed. It currently uses a python script to write the tests and test the rust functions.