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

Merg Releases/0.1.0 into dev #60

Merged
merged 61 commits into from
Jun 16, 2021
Merged

Merg Releases/0.1.0 into dev #60

merged 61 commits into from
Jun 16, 2021

Conversation

uvchik
Copy link
Member

@uvchik uvchik commented Jun 15, 2021

Both branches (dev and 0.1.0) have nothing to do with v0.0.12 and are not API stable. So we could go on with the dev branch to avoid confusion.

I do not add any additional code in this PR. I would ignore the decreasing coverage and open an extra PR to increase the test coverage.

gnn and others added 30 commits January 16, 2020 14:04
The module docstring, the main function with its docstring and the
imports necessary to declare all of this are put under version control.
This helps to differentiate the actual code moved from other modules
from the surrounding code wrapping the moved code into a function and
documenting it.
The code only got moved, `self.series` got replaced with `timeseries`
and some indentation got changed according to the new layout.
Nothing else should have changed.
First, using parenthesis instead of square brackets means instantiation
and is wrong. Second, specifying a `tuple` type should be done via
`typing.Tuple` and last but not least, the return type annotation via
`->` was missing.
The code previously worked on the `series` dictionary as a whole, but
the type annotation and the documentation hinted at the fact that
`deduplicate` only works on one list, i.e. on dictionary value, at a
time. Doing so makes the code more compact and doesn't complicate the
`deduplicate` call too much.
It used a very general type variable, where the type it expects is
actually quite specific. In order to fix that, this commit adds an alias
for this specific type and uses that instead of the type variable.
The multiples are themselves stored in lists containing consecutive
values which are deemed equal.
These values are compressed to pairs of the slice they cover and the
value they represent.
We have to go in reverse because replacing a covered slice with a single
value shortens the sequence, invalidating the indices of the other
slices. That's why we have to start with the last slice.
Not being able to compress all duplicates into one value means that some
values are beyond the desired margins of error, in which case raising an
error is probably the right choice.
The final release of the open_FRED weather data has been moved to the
"climate" schema, so that's where we should look for it by default from
now on.
Using `None` worked with the "oedialect" but didn't work when connecting
to the PostgreSQL database via "psycopg2". Also, using `True` makes more
sense because a filter which is always `True` doesn't filter anything,
which is what we want here.
@uvchik uvchik requested review from gnn and p-snft June 15, 2021 18:18
@uvchik uvchik self-assigned this Jun 15, 2021
Copy link
Member

@p-snft p-snft left a comment

Choose a reason for hiding this comment

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

I agree to continue as suggested.

@uvchik uvchik merged commit de067d2 into dev Jun 16, 2021
@uvchik uvchik deleted the releases/0.1.0 branch June 16, 2021 08:14
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

3 participants