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

Rl cleanup #20

Merged
merged 38 commits into from
Nov 1, 2019
Merged

Rl cleanup #20

merged 38 commits into from
Nov 1, 2019

Conversation

Rlamboll
Copy link
Collaborator

Pull request

Please confirm that this pull request has done the following:

  • Tests added
  • Documentation added (where applicable)
  • Example added (either to an existing notebook or as a new notebook, where applicable)
  • Description in CHANGELOG.rst added

Adding to CHANGELOG.rst

Please add a single line in the changelog notes similar to one of the following:

- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Added feature which does something
- (`#XX <https://github.com/znicholls/silicone/pull/XX>`_) Fixed bug identified in (`#YY <https://github.com/znicholls/silicone/issues/YY>`_)

@codecov
Copy link

codecov bot commented Oct 28, 2019

Codecov Report

Merging #20 into master will increase coverage by 3.83%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   95.22%   99.05%   +3.83%     
==========================================
  Files           9       10       +1     
  Lines         314      319       +5     
  Branches       52       53       +1     
==========================================
+ Hits          299      316      +17     
+ Misses         11        1      -10     
+ Partials        4        2       -2
Impacted Files Coverage Δ
...one/database_crunchers/quantile_rolling_windows.py 100% <100%> (+12.5%) ⬆️
src/silicone/cli.py 100% <100%> (ø) ⬆️
src/silicone/utils.py 100% <100%> (ø) ⬆️
src/silicone/stats.py 100% <100%> (ø)
src/silicone/plotting.py 95.23% <90%> (+2.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70f4448...eaeffbc. Read the comment docs.

@znicholls znicholls self-requested a review October 28, 2019 23:36
Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

Looks good @Rlamboll. There's a few things I'd tweak so I'll make a few PRs so we can discuss!

@znicholls znicholls mentioned this pull request Oct 29, 2019
This was referenced Oct 29, 2019
Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

lgtm. Only last question. What should happen if your x and y are the same array ie you do the quantile analysis on two identical datasets? You won’t get a linear interpolation back out will you? Rather steps?

@Rlamboll
Copy link
Collaborator Author

It's in steps, yes, as it will only return values found in the y dataset. You can see the answer to this question if you've ever run the plotting.py function (/script that runs this) on CO2 vs CO2 with quantiles on - it's pretty linear and as expected in the center of the distribution for all reasonable quantiles, but at the edges of the data there's less data so quantiles are pulled in the direction of the average value and are notably noisy.

@znicholls
Copy link
Collaborator

It's in steps, yes, as it will only return values found in the y dataset. You can see the answer to this question if you've ever run the plotting.py function (/script that runs this) on CO2 vs CO2 with quantiles on - it's pretty linear and as expected in the center of the distribution for all reasonable quantiles, but at the edges of the data there's less data so quantiles are pulled in the direction of the average value and are notably noisy.

Is it worth adding a test for this? Or putting in one of the example notebooks? The reason I ask is that if you use the infiller to infill a timeseries you already have, you won't get back what you put in which can be confusing so it would be good to document why.

@Rlamboll
Copy link
Collaborator Author

Rlamboll commented Oct 30, 2019 via email

@znicholls
Copy link
Collaborator

Ok I think it would be good to explain that in a notebook then. We can either do that in this PR or we can open an issue and then do it in a later PR, I'm happy either way (and conscious of time).

@Rlamboll
Copy link
Collaborator Author

OK, I've added stuff to the notebook to demonstrate the quirks of the windows value and retooled a test to check that you get out what you put in when there's nothing to average over. I've also removed the warning message.

CHANGELOG.rst Outdated Show resolved Hide resolved
src/silicone/stats.py Outdated Show resolved Hide resolved
src/silicone/stats.py Outdated Show resolved Hide resolved
tests/unit/test_stats.py Outdated Show resolved Hide resolved
@znicholls
Copy link
Collaborator

@Rlamboll I also didn't understand the last part of the notebook. Can you please add some more explanatory plots (or we make an issue and deal with it later).

@znicholls
Copy link
Collaborator

@Rlamboll once we've dealt with #24 and the review comments let's merge above. We can leave the notebook for #25

Lamboll and others added 6 commits October 31, 2019 10:22
Co-Authored-By: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org>
Co-Authored-By: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org>
Co-Authored-By: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org>
Co-Authored-By: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org>
@znicholls znicholls self-requested a review October 31, 2019 21:45
Copy link
Collaborator

@znicholls znicholls left a comment

Choose a reason for hiding this comment

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

Looks good @Rlamboll. One suggested extra comment which I think would be helpful otherwise good to go (after rebasing)!

@Rlamboll Rlamboll merged commit eaeffbc into master Nov 1, 2019
@Rlamboll Rlamboll deleted the RL-cleanup branch November 1, 2019 00:35
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.

2 participants