MICRO 2021 AE Paper #12 Reviews and Comments =========================================================================== Paper #12 SquiggleFilter: An Accelerator for Portable Virus Detection Comment @A1 by Reviewer B --------------------------------------------------------------------------- Dear authors, Could you provide an access to a jupyter notebook for the software evaluation ? Despite my efforts, I can't execute all cells correctly. I have an issue with the hdf5 library. The pyh5 embedded library in the virtual environment keeps trying to open files in /usr/local/hdf5/lib/plugin and there is nothing here. I tried with 3 different machines, it's always the same issue. Thank you. Comment @A2 by Reviewer A --------------------------------------------------------------------------- I can confirm that hdf5 error happens in my setup too. In cell 11. Comment @A3 by Reviewer A --------------------------------------------------------------------------- After a few steps, I was able to run the notebook: - `pip install hdf5plugin` - manually download `vbz` from github https://github.com/nanoporetech/vbz_compression/releases/download/v1.0.1/ont-vbz-hdf-plugin-1.0.1-Linux-x86_64.tar.gz - move the `vbz` shared library into the hdf5plugin folder, for me it's `lib/python3.6/site-packages/hdf5plugin/plugins` - set the HDF5_PLUGIN_PATH env virable to the hdf5plugin folder before starting the notebook And a couple minor suggestions in the `setup.sh` script: - aws needs to be install with sudo, or with `-i` and `-b` option - add `mkdir img` in the script so that the notebook does not fail when saving the images Review #12A =========================================================================== * Updated: Sep 3, 2021, 5:17:05 PM UTC Reviewer Expertise ------------------ 2. Some familiarity Artifact Publicly Available --------------------------- 2. Yes Artifact Functional ------------------- 2. Meets expectation Key Results Reproduced ---------------------- 1. Below expectation Distinguished Artifact Nomination --------------------------------- 1. No Comments for PC --------------- This artifact reproduces many of the results in the original paper instead of just one or two figures. The evaluation includes both software and hardware (simulation) which is overall well prepared. However I am not an expert in accelerator designs thus have not much insight into the complexity involved. Review #12B =========================================================================== * Updated: Sep 3, 2021, 8:21:50 PM UTC Reviewer Expertise ------------------ 2. Some familiarity Artifact Publicly Available --------------------------- 2. Yes Artifact Functional ------------------- 2. Meets expectation Key Results Reproduced ---------------------- 1. Below expectation Distinguished Artifact Nomination --------------------------------- 1. No Comments for author ------------------- # Artifact availability: 2 (met expectations) The artifact is hosted on two platforms: Zenodo and Github. However, I have some comments on this: * the size of the artefact is ~700MB and it is mainly due to the "lambda" dataset that is embedded in the archive. Would it be possble to remove this dataset and upload it somewhere else ? * the archive "verilog.zip" contains plenty of garbages folders and files, especially vivado error logs, journals and backup. Please remove them for a proper artifact. # Artifact functional badge: 1 (fell below expectations) The artifact is composed of two distinct parts: a software part for computation that relies on Python, and an hardware part for VHDL simulation that relies on Vivado. Both parts do not run "as is". ## Vivado For the hardware part, it is due to a wrong project configuration that can be easily modified (right click on design sources -> hierarchy update -> automatic update and compile order). Without this, the synthesis failed and the simulation never start. In the paper, authors mentioned this file: "design/Hardware_README.txt". It does not exists. ## Jupyter notebook For the software part, a lot of things are incorrectly done and I suspect that authors have never tried their own installation script on their machine. The script setup.sh is supposed to do 4 things: * Step 1: download and install the Amazon Web Service command line interface * Step 2: download a public dataset somewhere in the UK * Step 3: download a dataset using the AWS API previously installed * Step 4: setting up a Python virtual environment for the computation Comments: * Step 1: the installation of AWS failed because authors do not use the option to install this tool without the sudo rights (-i and -b). Do you expect me to run a script with sudo from an unknown source ? * Step 3: downloading the dataset on AWS failed because the tool was not installed during step 1 * Step 4: the command for setting up the virtual environment uses an hard-coded version of python (3.6). It fails on my machine since I have python 3.7. I don't understand why authors do not use a command to check which version of python is installed before setting up the environment. All along the script, there is no error management. It tries to do step 3 while step 1 failed. During step 4, the availability of python3.6 is not checked and it tries to install a virtual environment. In conclusion, for the installation script, only 1 step on 4 is correctly done and I had to redo the 3 others manually. Moreover, the Python code expects an "img" folder to save plots generated during the computation. This folder is not created during the installation phase, which leads to an error during the computation. Regarding the computation, the python virtual environment to execute the Jupyter notebook is not working either. There is an issue with a missing library in requirements.txt (hdf5plugin), and the environment variable HDF5_PLUGIN_PATH is not set. As a result, the computation fails after few hours. # Artifact reproducible badge: 1 (fell below expectations) ## Vivado Nothing to report. ## Jupyter notebook The computation phases generates 11 figures from the paper. However, only 4 figures depicts actual results of the paper. I wonder why authors have included them in the artifact. - Figure 11: results with 1000 samples drastically differs from the paper. Authors claims that only a small part of human DNA samples are incorrectly below the threshold. In my case, half of the human samples are incorrectly classified - Figure 17a: due to the previous results I obtained with 1000 DNA samples, the accuracy for the blue curve is almost zero. - Figure 17b: impossible to reproduce the figure with the Python code - Figure 17c: due to the previous result with 1000 samples, results with 1000 samples drastically differs. Moreover, the selected threshold with 2000 and 3000 samples differs with a significant gap - Figure 21: different from the paper but trends are respected, no issue.- - Figure 2, 5, 10 : reproducing these figures is irrelevant and is not part of the artifact evaluation. - Figure 16a and Figure 16b: results for these figures comes from emulation on Amazon Web Service. Therefore, they are are note reproducible during the review (which is 100% normal). Why authors include these results in the artifact evaluation process ? - Figure 18: results to produce this plot are embedded in the artifact and does not come from the computation. I wonder why since it shows the accuracy deviation from the standard implementation of the sWDT algorithm. This implementation is run into the Jupyter notebook and its results stored in scripts/results/artifact-evaluation/fscores.npy. If results for this plot must be obtained by using AWS simulation, it is fine for the reviewer to not reproduce the figure. Hence, why authors included this plot in the artifact evaluation process ? - Figure 19: same comment as Figure 18. Comments for PC --------------- - the artefact is barely functional - it produces 11 figures but only three of them are real results (11, 17a, 17c), one is not reproducible (17b), and the last one (21) is not very relevant - the hardware part is irrelevant since no results are extracted with this computation. It is just a Verilog code that runs into the IDE. By quickly looking at the code, it is impossible to say if it works or not. It just runs until the end. Comment @A4 by Reviewer A --------------------------------------------------------------------------- Hi authors, - Please update your artifact to generate Figure 17b and 17c at the same time, instead of controling figure generation using the 'virus' variable. - Please replace the hard-coded `best_threshs # measured from lambda phage` with computed threasholds from Figure 17b. The hard-coded thresholds are not optimal for the reviewers' random subset sampled from the entire dataset. - Please use a deterministic seed for the random number generator for reproducibility. Comment @A5 by Author [Timothy Dunn ] --------------------------------------------------------------------------- Reviewers, First of all, thanks for the feedback and I apologize for the slow response. I was out of the office all of last week. I've managed to replicate the HDF5 library errors on a new machine, and adding `import ont_fast5_api` to the first cell should fix the issue. Please let me know if it does not. I'm working on updating the artifact to use a random seed, removing the hard-coded thresholds, and regenerating both Figures 17b and 17c simultaneously. I should be able to push the updated notebook (and the fixed `setup.sh` script) some time tomorrow morning. Thanks for your patience! Comment @A6 by Author [Timothy Dunn ] --------------------------------------------------------------------------- All changes have been pushed to the Github repository. Please let me know if you run into any issues, or if you would still prefer access to a preconfigured notebook. Comment @A7 by Author [Timothy Dunn ] --------------------------------------------------------------------------- Reviewer B, It looks like you may have missed our earlier response with fixes on 8/24. I was unfortunately out of the office the week of 8/16, when you originally posted comments. ### Artifact Availability: **Vivado:** The unnecessary Vivado log files and directories have been removed for our final artifact. **Jupyter Notebook:** We originally kept the `lambda` dataset within our repo for ease-of-setup, but we can move the `lambda` dataset to Git LFS if you believe that is a better solution. ### Artifact Functionality: **Vivado:** Please note that the hardware functionality can be verified “as-is”. Our project included in the artifact is intentionally configured only for the functional verification of our hardware design and not for ASIC synthesis. Hence, if you follow “Loading the environment” on `README.md`, the SystemVerilog testbench (`testbench_top.sv`) is rightly set as the default top file. The README has updated to make our intention clearer. Most importantly, to functionally verify the hardware, please refer to “Behavioral simulation using the testbench” in `README.md`. This subsection has been updated with additional details to help guide waveform inspection. However, we will not be able to share or provide direct access to our ASIC synthesis setup due to a licensing contract between U Michigan and TSMC for 28nm libraries. You need to be an employee of UM to directly access it. Hence, ASIC synthesis setup is not included in our artifact submission. However, if you still wish to do this, please reach out back to us, and we can set up a Zoom meeting and let you view our synthesis setup and run. `Hardware_README.txt` was merged with `README.md`. We have corrected this line in the paper. **Jupyter Notebook:** Many of the comments here have already been addressed. Yes, an import statement, additional `sudo` flags, and an `img` output directory were missing from our original artifact. However, those minor issues have since been fixed and we’ve improved the error-checking in our pipeline. Please note that the MICRO reviewing guide states: *our philosophy of artifact evaluation is not to fail problematic artifacts but to help the authors improve their artifacts (at least publicly available ones) and pass the evaluation!* ### Artifact Reproducibility: **Jupyter Notebook:** At only 1000 samples (Fig 11 and blue line in Fig 17a), it is expected that a large proportion of reads are misclassified. If your plots significantly differ from those in our paper, can you please share them and/or the seed you used? We have seen consistent results during our evaluations. Your suggested improvements regarding Figures 17b and 17c were incorporated previously, with recomputed thresholds and simultaneous plotting. You rightly question why several figures are included in the artifact evaluation; we decided to keep them under the subheading “Other Figures” (even if they include results calculated elsewhere), for completeness and because the code in several cases documents additional detail as to how our calculations were performed. If you still believe that they are unnecessary, we can remove them.