Closed dylan-asmar closed 7 months ago
@dylan-asmar this is really great! Thank you for the contribution! Thanks especially for adding tests.
If you want to continue this, it might be nice to get convert(SparseCat, b)
to work too.
(btw, in general it is better to avoid adding whitespace changes to PRs (unless the PR is about whitespace changes), but I know that sometimes editors do that automatically, so it is difficult to avoid)
@zsunberg , I was implementing convert
like you mentioned, but stumbled upon
Conversion vs. Construction Note that the behavior of convert(T, x) appears to be nearly identical to T(x). Indeed, it usually is. However, there is a key semantic difference: since convert can be called implicitly, its methods are restricted to cases that are considered "safe" or "unsurprising". convert will only convert between types that represent the same basic kind of thing (e.g. different representations of numbers, or different string encodings). It is also usually lossless; converting a value to a different type and back again should result in the exact same value.
in the Julia documentation. DiscreteBelief
has a reference to the pomdp as one of its fields. Therefore, when we create a SparseCat
out of it, it drops this information. So this process feels more like a construction to me than a conversion.
Thoughts?
@dylan-asmar This is a good question to ask.
I would consider conversion between DiscreteBelief
and SparseCat
to be conceptually lossless because the only function of the pomdp in DiscreteBeleif
is looking up the state index (I think). According to the distribution interface (i.e. rand
, pdf
, etc.), the two distributions would behave exactly the same (except for maybe wrt the random number seed).
Unfortunately, right now it is impossible convert
a SparseCat
to a DiscreteBelief
since you need a pomdp to construct the DiscreteBelief
. However, it would be relatively easy to generalize DiscreteBelief
to not need the pomdp by changing the pomdp field to a state index lookup function that defaults to s -> stateindex(pomdp, s)
. (there may be other factors that I am not seeing)
In any case, even if we don't modify DiscreteBelief
, I think it does make sense to have a convert
method for DiscreteBelief
to SparseCat
.
I found myself converting DiscreteBeliefs to SparseCat distributions often so I could visualize the distribution using the default UnicodePlots
show
function built-in. This change just extends the sameshow
function toDiscreteBelief
.Current:
New:
I have also found myself converting from DiscreteBeliefs to SparseCats for various reasons (probably not great ones outside of visualizations...but they existed). I added a constructor for
SparseCat
that accepts aDiscreteBelief
as the input instead of the names and probabilities.Current:
New:
Other changes in the files is just removing whitespace.