search

normal-computing / thermox

Exact OU processes with JAX
Apache License 2.0
24 stars 3 forks source link

rename function to flag internal use #8

Closed KaelanDt closed 2 months ago

SamDuffield commented 2 months ago

I don't feel strongly on this, the sample_identity_diffusion function is already kinda not public facing since it only lives in thermox.sampler.sample_identity_diffusion unlike thermox.sample.

Happy to add the underscore if we like but if we do we should also add it to thermox.log_prob.log_prob_identity_diffusion to be consistent.

KaelanDt commented 2 months ago

I don't feel strongly on this, the sample_identity_diffusion function is already kinda not public facing since it only lives in thermox.sampler.sample_identity_diffusion unlike thermox.sample.

Happy to add the underscore if we like but if we do we should also add it to thermox.log_prob.log_prob_identity_diffusion to be consistent.

That's true, let's keep it that way. Thinking about it again, I just don't like that sample_identity_diffusion can throw an error if you want to use it on a GPU, so what do you think of simply adding A_spd as a kwarg to it?

SamDuffield commented 2 months ago

I don't feel strongly on this, the sample_identity_diffusion function is already kinda not public facing since it only lives in thermox.sampler.sample_identity_diffusion unlike thermox.sample. Happy to add the underscore if we like but if we do we should also add it to thermox.log_prob.log_prob_identity_diffusion to be consistent.

That's true, let's keep it that way. Thinking about it again, I just don't like that sample_identity_diffusion can throw an error if you want to use it on a GPU, so what do you think of simply adding A_spd as a kwarg to it?

Yes absolutely!! Same wavelength, I'd even made an issue for it #9 😆

we should also do it for thermox.log_prob.log_prob_identity_diffusion

SamDuffield commented 2 months ago

Can we unify the docstring for A_spd for log_prob too?