Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation/rli datasets #1138

Merged
merged 79 commits into from Aug 31, 2023
Merged

Documentation/rli datasets #1138

merged 79 commits into from Aug 31, 2023

Conversation

birgits
Copy link
Contributor

@birgits birgits commented Jul 24, 2023

In this branch the following datasets are documented:

  • CtsDemandBuildings
  • Demand_Building_Assignment
  • HeavyDutyTransport
  • Household Demands
  • LoadArea
  • MITChargingInfrastructure
  • MastrData
  • MotorizedIndividualTravel
  • MvGridDistricts
  • OpenStreetMap
  • OsmBuildingsStreets
  • RePotentialAreas
  • VG250
  • Vg250MvGridDistricts
  • ZensusMvGridDistricts

As some datasets were created using partial and documenting them was not possible, their setup was changed as well with PR #1146.

gnn and others added 22 commits August 9, 2023 18:26
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.
…ace-partial-with-class

Replace `partial` applications of `Dataset` with `Dataset` subclasses
Namely `egon.data.datasets.Dependencies` and `egon.data.datasets.Tasks`.
Hopefully this makes the documentation of dataset subclasses using
`@dataclass` less confusing.
Solution taken from a helpful [answer] on stackoverflow.

[0]: https://stackoverflow.com/a/67483317
At least all those, which Sphinx highlights in boldface.
IMHO it's more readable this way. Feel free to remove the commit if you
don't concur.
By removing an unnecessary level of indentation.
Courtesy of `black`.
Instead of relying on `@dataclass` to provide one. This makes the
constructor's signature less confusing. I also allows not specifying
types for the class variables, because `@dataclass` no longer needs to
pick the up automatically, as we are manually specifying them in the
superclass constructor call.
This means we have less stuff to import, most notably we no longer need
to import `Tasks`. This also allows us to work around the broken display
of `mv_grid_districts_setup`'s `tasks`. That was only a single function
and for some reason Sphinx did not display that one correctly. Probably
because it tried to use `str` instead of `repr` to render the function,
because boxing the function in a one-tuple would fix the display.
Anyway. Not putting the task on a class attribute but specifying it
directly in the constructor means that no class attribute is displayed
in the documentation, thus sidestepping the issue.
…ted with partial

Propose alternative replacements for partial.
src/egon/data/datasets/emobility/heavy_duty_transport/h2_demand_distribution.py
@birgits birgits marked this pull request as ready for review August 14, 2023 11:49

* explain scenario parameters

* run params (all in meta file?)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question mark looks a bit strange...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it!

Copy link
Contributor

@ClaraBuettner ClaraBuettner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the small comment related to the question mark, this branch looks good to me.

@birgits birgits merged commit 312d71f into dev Aug 31, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants