-
Notifications
You must be signed in to change notification settings - Fork 54
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
JOSS review #76
Comments
Hey @theorashid, thanks for giving us such a helpful set of comments. I'll address them in order below.
The API page has a right hand menu which is structured as
I do not have a great deal of experience with NumPyro. We chose to offload MCMC sampling into BlackJax and optimisation to Optax as they were both quite "research focussed" packages which aligned with the principles in GPJax. I'm curious, would you mind expanding on the advantages that NumPyro integration would give in comparison to Optax and BlackJax?
Good point - this has been resolved here.
Cool suggestion. A pre-commit automation that uses the black formatter has been added here.
This is on our radar. As is a guide detailing custom implementation. We would like to give it a bit more thought though as our end goal is to have an API such that one can simply inherit a Distrax distribution and define one or two methods.
A major reason for jejjohnson's large number of committed lines of code is that we used |
Many of those points are fair and I've ticked them off. I don't have much experience with pre-commit but make sure it runs for every PR, and maybe add it to the CONTRIBUTING.md. Glad it was easy enough to implement.
Ah okay, I didn't notice the toggle for the menu in the top right. I see it now and this is all good. If there's a way to set it as open by default, that would be great.
It's more for the cases when you want to use GPs as part of a larger model. In spatial modelling, perhaps you only want a Gaussian process over space in a larger model with several covariates added as As for NumPyro specifically, I just prefer using it for model building to tfp. And it's written in jax. So, taking from your tfp example, it would be nice if you could replace -priors["kernel"]["lengthscale"] = tfd.Gamma(concentration=jnp.array(1.0), rate=jnp.array(1.0))
+priors["kernel"]["lengthscale"] = numpyro.sample("rho", numpyro.dist.Gamma(1.0, 10.0)) and equally the numpyro mcmc interface is much simpler imo than tfp with it's confusing Personally, I would always use blackjax for mcmc over tfp, especially with their new API.
I guess you could look towards how the tingp library has done this with numpyro. |
I will investigate this.
Yes, very cool. I really like this idea. If you were interested, I'd be very interested to talk more about the logistics of doing this with a nice real-world example in GPJax. To your points on NumPyro and likelihood design. I think to do them cleanly and correctly will be quite a large piece of work. Over the next month, I'm away at conferences quite a bit and writing up my thesis, so will struggle to properly sit down and work out how best to do this. How much of a priority are they to you with regards to accepting GPJax? If they're crucial, then I can look at re-jigging my schedule a little to make time for them. |
Sorry if it wasn't clear: only the two points on the checklist at the top and the paper were essential to accepting the submission. In my view, the codebase is more than ready to be accepted – I just need to read and edit the latest version of the manuscript. The numpyro stuff and menu are not essential at all. Obviously there's another reviewer but great job.
I'd be really interested – particularly for my own work. I think a spatiotemporal problem would be a nice way to involve that community with your package. A good example dataset would be the Hungary Chickenpox dataset . I would like to do this with a graph GP over space kroneckered with a GP over time (see Fast Hierarchical GPs) if that's possible. It's also a Poisson likelihood and we could try and add a covariate in for a larger model. Perhaps when you're more free, we could sit down for a chat about implementing this – many in my spatiotemproal epidemiology crowd would be interested. Also, leave this issue open for now and I'll put any comments I have about the paper here before closing it. |
Great. Very interested to explore this more, particularly in relation to the graph component with a temporal component. I have opened a discussion thread #80 for us to continue publicly discussing this idea further.
Sure! Look forward to anymore thoughts you have on the package. |
Paper looks in good shape. Notes:
|
Hey @theorashid. I've just update the paper to reflect your most recent comments. My labmate works exclusively in R and tells me all of his GP code is written from scratch. Besides Stan, he was also unaware of any popular R packages for GPs. With that in mind, I've left the R acknowledgements to the GPStuff port that you've flagged above. |
Looks like a minor typo "theGaussianProcesses,jl". Worth doing a final check that the references are rendering as you expect too Once that is fixed, I think we just need to wait for the other reviewer's comments. |
Hey @theorashid. Many thanks for your constructive comments. The NumPyro idea is cool! @theorashid, @thomaspinder, I have conducted a pass through the paper with fresh eyes and have condensed it down. All changes are on the master branch (apologies - forgot to create a PR - the commits in order are 6c26920, dc20335 & 11806f6). The major change was collapsing the @thomaspinder, please are you able to correct the formatting of |
@theorashid Nice catch. The typo has been fixed above by Daniel. @daniel-dodd happy with your changes and the references are now updated, as per your request. |
Hi @thomaspinder and @daniel-dodd, here's some additional items from my review:
Overall, I'm really excited about this library, excellent work! I think the things marked as issue are important, but everything else is merely suggestion. |
Thanks @gpleiss for such helpful comments! I’m currently away at ISBA, so I‘ll address your comments on Monday once I’m home. Having read your comments though, I’m confident that we can resolve them all promptly. |
Hi @gpleiss.
Thanks again for providing helpful comments as a reviewer. |
Thanks for addressing my comments!
Hmm... I personally haven't noticed a headache with Torch/GPyTorch, and I don't think we've ever had any GitHub issues about cuda installation. There probably is a better pattern out there for cuda installation, but I don't think that this project has to figure it out 😄
Oops! I must have missed this. The GitHub action is exactly what I had in mind.
The notebook looks awesome! I really like the diagnostic plots (and I am definitely stealing the idea for our gpytorch tutorials 😉)
Awesome! |
This is for openjournals/joss-reviews#4455.
Thanks for the submission – it's a nice package with a clean codebase. Beyond some edits to the script, for which I opened a PR, there are some other points to consider (happy to discuss).
Before accepting:
Not essential, but would be nice:
API which you can click through on the menu bar rather than one big dump at https://gpjax.readthedocs.io/en/latest/api.htmlit would be good to automate linting and formatting, ideally using a pre-commit and a workflowThe text was updated successfully, but these errors were encountered: