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

Adding plot_kernel() function to utils #13

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Adding plot_kernel() function to utils #13

wants to merge 6 commits into from

Conversation

AndrewILWilliams
Copy link
Collaborator

@AndrewILWilliams AndrewILWilliams commented Feb 9, 2021

Example usage below. Hoping this will make exploring different kernels a bit easier for users.

image

To-do:

  • [] Add tests

@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

Merging #13 (04a4093) into master (95613d2) will decrease coverage by 0.21%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
- Coverage   97.03%   96.81%   -0.22%     
==========================================
  Files           7        7              
  Lines         438      440       +2     
==========================================
+ Hits          425      426       +1     
- Misses         13       14       +1     
Impacted Files Coverage Δ
GCEm/__init__.py 96.42% <80.00%> (-1.14%) ⬇️

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 95613d2...04a4093. Read the comment docs.

@duncanwp
Copy link
Owner

Nice, thanks! I recently factored the kernel creation in to a separate function (_get_gpflow_kernel), could you use that instead of replicating it here?

I'm a little uncomfortable with the lack of test coverage in utils.py too. Testing plots is a pain but maybe we should split the plotting routines in to a separate file?

@AndrewILWilliams
Copy link
Collaborator Author

Nice idea! I'll look into that now

In terms of tests I was thinking primarily of checking that the assertions fail correctly (if the user gives an incorrect kernel name) and maybe also checking that the axes output has the right shape? Currently I've set it up so it gives an adaptive number of subplots depending on the input kernel list and whether you also want a "Product"/"Sum" plot as well

@AndrewILWilliams
Copy link
Collaborator Author

@duncanwp I've made a small change to _get_gpflow_kernel so that it can also return the list of individual kernels if asked to. Now calling this in the kernel_plot() function rather than reproducing that code

@duncanwp
Copy link
Owner

I like the idea but I'm not a big fan of optionally returning a different number of arguments, it can make code a bit hard to visually debug. Looking at it again maybe get_kernel should just initialise a single kernel, and there should be a separate (simple) function for combining them?

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