Skip to content

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