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

Move post_init check into eval for kernels which provide it. #148

Merged
merged 8 commits into from Mar 11, 2023

Conversation

femtomc
Copy link
Contributor

@femtomc femtomc commented Jan 20, 2023

@dfm Sorry -- I SNAFU'd my other PR on accident with my poor local git abilities.

This is a continuation of #146 -- and moves the post_init check into evaluate for kernels which perform the check.

Re -- I'm not sure if my moving of assert ... until after the checks will mess up typechecking -- let me know if you end up having a moment.

@femtomc
Copy link
Contributor Author

femtomc commented Jan 29, 2023

Bumping this -- in case a maintainer can run the approved workflows (which I don't have access to).

@dfm
Copy link
Owner

dfm commented Jan 29, 2023

Whoops - sorry for being slow! I've had a crazy couple of weeks. Thanks for the ping.

@femtomc
Copy link
Contributor Author

femtomc commented Jan 30, 2023

@dfm Ah, so it looks like I need to change some of the tests - where you explicitly check for the failures I've moved.

@femtomc
Copy link
Contributor Author

femtomc commented Mar 8, 2023

Hi @dfm -- sorry for the delay on finishing this (research and all). How do you want to handle Product checks -- e.g. here:

    # Check for dimension issues when multiplied and evaluated.
    with pytest.raises(ValueError):
        k = jnp.ones(3) * kernels.Matern32(1.5)
        k.evaluate(v, v)

with this change, users are allowed to create k - but evaluation will throw an error.

And, just to remind ourselves why I motivated this change (it's been a few weeks) -- sometimes I want to return kernels out of vmap calls (e.g. imagine returning a trace from a probabilistic program where we also record what kernel was invoked), which means that they should evaluate just fine, but dimensional checks on construction will fail for this pattern.

@dfm
Copy link
Owner

dfm commented Mar 8, 2023

@femtomc — Absolutely no worries! This looks like a good approach, the only change I would make would be to isolate the test, i.e.:

    # Check for dimension issues when multiplied and evaluated.
    k = jnp.ones(3) * kernels.Matern32(1.5)
    with pytest.raises(ValueError):
        k.evaluate(v, v)

to make it a bit clearer. Otherwise I'm happy with this!

@dfm
Copy link
Owner

dfm commented Mar 8, 2023

The docs failure isn't related to this PR - can you rebase when you get a chance?

For the "news" failure, can you add a file called news/148.feature that briefly describes this PR? The text will appear in the release notes.

Finally: can you also add your info to the .zenodo.json file so that you can be included in the author list for the Zenodo release?

@femtomc
Copy link
Contributor Author

femtomc commented Mar 10, 2023

Thank you! Hopefully all done.

@femtomc
Copy link
Contributor Author

femtomc commented Mar 10, 2023

Oops -- I missed your comment about isolating the test. Will fix.

femtomc and others added 3 commits March 9, 2023 23:03
@dfm dfm merged commit fcdd800 into dfm:main Mar 11, 2023
9 checks passed
@dfm
Copy link
Owner

dfm commented Mar 11, 2023

Thanks @femtomc!! 🚀

@femtomc femtomc deleted the mrb/move_eval_check branch March 11, 2023 18:49
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.

None yet

2 participants