Simplify file compression #818
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmbenlloch
approved these changes
Jun 29, 2022
Contributor
jmbenlloch
left a comment
jmbenlloch
left a comment
There was a problem hiding this comment.
Good job! The code is more consistent now and less prone to errors. I like your test_city_output_file_is_compressed test. Approved!
This test ensures all cities produce an output file that is fully compressed.
We missed it when the city was introduced
This allows data compression
The scope of this change will be clear in the following commits
This doesn't mean that the tables will not be compressed. Instead, the compression configuration will fall back to a default value coming from the overall file compression. However, if a non-None value is passed, this will force that specific compression for the corresponding nodes. This still allows for the (somewhat rare) cases when an user wants to use these writers outside of a city. From now on, *cities are responsible for setting the compression configuration on file opening*
This avoids confussion since the compression for each node is now inherited from the overall file compression.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR, I revisit the file compression in IC. The main point was that this feature relied on the same default value being used everywhere (namely
"ZLIB4"). While addressing this, I noticed that we didn't have any check for compression and that we were doing it inconsistently. Furthermore, the compression options were redundant, as the compression library and level can be set at file opening, and the data stored inherits this configuration.There are two significant changes implemented here:
These changes won't have a major impact on users or developers. Nonetheless, developers writing new cities will have to be aware of this.