Replace partial applications of Dataset with Dataset subclasses
#1145
Conversation
This file contains 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
[PEP 8][0] actually specifies a maximum line length of 72 characters for "flowing long blocks of text with fewer structural restrictions (docstrings or comments)". I extended that to 76 characters for indented docstrings, because the reason for having 72 characters is so that the eyes don't have to travel horizontally too much. Indentation basically moves the start of the text block to the right, so for text blocks like this, the line length limit can move with the indentation. Some of these lines where even beyond 79 characters, so I had to touch them anyways to make `flake8` happy and while at it, I also changed them to be (mostly) PEP 8 compliant. [0]: https://peps.python.org/pep-0008/#maximum-line-length
Moving `OsmBuildingsSynthetic.n_amenities_inside.type` into a variable is done to work around a bug in `black`. Without this, `black` would always reformat the line to be exactly 80 characters, even if a limit of 79 is set, making `flake8` complain.
This helps when specifying subclasses of `Dataset`. Since there was already a `Tasks` class inside the module, that class got renamed to `Tasks_` because it could not be used for the purposes of the type.
Make these subclasses `@dataclass`es, just like the original `Dataset`, so that default values for the fields are picked up by the constructor. That way we don't have to specify a constructor that calls the superclass constructor, but can rely on the default implementation for `__init__`. The `dependencies=[]` line present in nearly all uses of `partial` has been removed because it was unnecessary. The `dependencies` parameter is usually specified when the datasets are instantiated and even if not, an empty list of dependencies is the default anyways. Actually, an empty tuple is the default, but that doesn't make a difference. The `tasks=(define_mv_grid_districts)` line has been changed to drop the parenthesis because the parenthesis where superfluous in this case. The `tasks=(download_mastr_data,)` line has not been changed, because even though `Dataset`s should handle a callable and a tuple containing exactly one callable identically for the tasks attribute, I didn't want to risk introducing any bugs.
birgits
approved these changes
Aug 10, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This should finally enable documenting those subclasses.