ESEm
ESEm copied to clipboard
Adding plot_kernel() function to utils
Example usage below. Hoping this will make exploring different kernels a bit easier for users.
To-do:
- [] Add tests
Codecov Report
Merging #13 (04a4093) into master (95613d2) will decrease coverage by
0.21%
. The diff coverage is80.00%
.
@@ 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%) |
:arrow_down: |
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.
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?
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
@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
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?