Update codec validation plots - #64
Conversation
tovrstra
left a comment
tovrstra
left a comment
There was a problem hiding this comment.
A few comments regarding one technicality to make it easier to understand the code.
The general idea of simplifying the plot is a good one. The change in RMSE calculation (between one overall RMSE or an RMSE of subsets averaged over seeds) should be small, but it conceptually aligns better with how the rest of the dataset works, which is good too.
| rmse_ref_per_resolution = data["rmse_ref_per_resolution"].item() | ||
| rmse_raw_per_resolution = data["rmse_raw_per_resolution"].item() | ||
| rmse_raw_per_seed = data["rmse_raw_per_seed"] | ||
| rmse_codec_per_seed = data["rmse_codec_per_seed"].item() |
There was a problem hiding this comment.
I'd suggest to tackle this differently. I had to add some debug lines to understand why the .item() was needed.
It would be good to rule out pickling when saving NPZ files. With pickle, about anything can be stored in an NPZ file, making it also harder to understand what is being stored. I'll add a second comment with some more direction, but I'd generally recommend allow_pickle=False for all savez() calls in this repository, just to enforce simpler NPZ files. See https://numpy.org/doc/stable/reference/generated/numpy.savez.html
| rmse_raw_per_resolution[resolution] = np.sqrt(np.mean((psd_coded - psd_raw) ** 2)) | ||
| codecs = {resolution: generate_codec(resolution) for resolution in RESOLUTIONS} | ||
| rmse_raw_per_seed = np.zeros(NSEED) | ||
| rmse_codec_per_seed = {resolution: np.zeros(NSEED) for resolution in RESOLUTIONS} |
There was a problem hiding this comment.
This is the follow-up on the pickle comment. Because this line creates a dictionary, savez will pickle it as a singleton array containing a dictionary, which is a bit odd. There are other reasons to avoid pickle (very complicated format, overkill for this project, complexity in terms of understanding how it works, ...)
Practically, this can be done as follows:
- Turn
rmse_codec_per_seedinto an array, of which the rows correspond to different resolutions. - Store the
RESOLUTIONSarray into the NPZ, so follow-up scripts can use them.
This essentially splits the dictionary keys and values into two corresponding arrays, which can be stored in an NPZ without pickling.
|
Thanks for the feedback! I did an additional refactor to remove all pickled files in the other validations as well. |
2 participants
In this PR, the top panels have been removed from the codec validation plot, as they did not provide substantial additional insight.
Furthermore, the RMSE calculation has been revised. Instead of pooling all
256 * 64samples into a single RMSE estimate, RMSEs are now computed by pooling over the 256 sequences for each seed separately and repeating this across 64 seeds. The final reported RMSE values are then obtained by averaging over these 64 repetitions. This approach more closely reflects the intended use of ACID.Note that the equations in
validation.typhave not yet been updated to reflect these changes. This will be addressed in a large upcoming documentation PR.