Issue 561: Rename variables to reflect that they are choice maps

[SamuelBrand1]

This PR closes #561 .

This PR renames instances where a variable is a ChoiceMap to use choice_ prefix consistently, i.e. use 'choice_value' and 'choice_gradient' instead of 'value_trie' and 'gradient_trie' for clarity and alignment with choicemap terminology. I've left variables that are typed to be Tries.

Main danger is that my understanding of the static IR for Gen is that the gensyms for e.g value_trie are also meant to reflect creating a local variable name for a choice map object. If this is incorrect then I should revert those changes.

[ztangent]

Thanks for this! Everything seems to pass, but could we change choice_gradient to choice_grads? I'm a little worried that choice_gradient is a little too close to the name of the choice_gradients function itself. choice_grads is also what is used within the documentation of the choice_gradients function.

Your understanding of the static IR is correct!

[SamuelBrand1]

Thanks for this! Everything seems to pass, but could we change choice_gradient to choice_grads? I'm a little worried that choice_gradient is a little too close to the name of the choice_gradients function itself. choice_grads is also what is used within the documentation of the choice_gradients function.

Your understanding of the static IR is correct!

Thats a good point, I'll make a quick change.

[SamuelBrand1]

Having another look at the code base I found

Gen.jl/src/dynamic/backprop.jl

Lines 349 to 350 in 8f2f4cc

	(arg_grads, value_choices, gradient_choices) = choice_gradients(
	record.subtrace, record.selection, retval_grad)

this has the other way of representing the variables e.g. gradient_choices etc. I think it would be nice to lock onto one convention to make the src code easier to read?

[ztangent]

Yes, feel free to change that as well!

[ztangent]

Now that I've merged #559 into the main branch, it'd be great if you could resolve the merge conflicts with that as well.

[SamuelBrand1]

I've gone for value_choices and gradient_choices because those match the field names of the BackpropTraceRecord struct that is used in Gen backpropagation which I think helps make the code read slightly easier.

I've also changed this for the MAP opt and mala but I've let the unit tests be lax on these changes.

[ztangent]

Looks good but let's update the docstring and the static IR code as well.

[SamuelBrand1]

It was a bit fiddly because you don't want to collide with the choice_gradient function when refactoring to choice_gradients variable.

[ztangent]

Looks good to me, thank you!