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

Added show option to DiscreteBelief and extended SparseCat #529

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

dylan-asmar
Copy link
Member

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 same show function to DiscreteBelief.

Current:

julia> b = DiscreteBelief(MiniHallway(), [0.1, 0.1, 0.123, 0.4, 0, 0, 0, 0, 0.177, 0.05, 0, 0, 0.05])
DiscreteBelief{MiniHallway, Int64}(MiniHallway(Deterministic{Int64}[Deterministic{Int64}(1), Deterministic{Int64}(2), Deterministic{Int64}(7), Deterministic{Int64}(4), Deterministic{Int64}(1), Deterministic{Int64}(10), Deterministic{Int64}(1), Deterministic{Int64}(8), Deterministic{Int64}(9), Deterministic{Int64}(10), Deterministic{Int64}(13), Deterministic{Int64}(8), Deterministic{Int64}(13)]), [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13], [0.1, 0.1, 0.123, 0.4, 0.0, 0.0, 0.0, 0.0, 0.177, 0.05, 0.0, 0.0, 0.05])

New:

julia> b = DiscreteBelief(MiniHallway(), [0.1, 0.1, 0.123, 0.4, 0, 0, 0, 0, 0.177, 0.05, 0, 0, 0.05])
              MiniHallway DiscreteBelief         
      ┌                                        ┐ 
    1 ┤■■■■■■■■■ 0.1                             
    2 ┤■■■■■■■■■ 0.1                             
    3 ┤■■■■■■■■■■■ 0.123                         
    4 ┤■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 0.4   
    50                                        
    60                                        
    70                                        
    80                                        
    9 ┤■■■■■■■■■■■■■■■ 0.177                     
   10 ┤■■■■ 0.05                                 
   110                                        
   120                                        
   13 ┤■■■■ 0.05

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 a DiscreteBelief as the input instead of the names and probabilities.

Current:

julia> SparseCat(b)
ERROR: MethodError: no method matching SparseCat(::DiscreteBelief{MiniHallway, Int64})

Closest candidates are:
  SparseCat(::Any, ::AbstractArray{<:Number})
   @ POMDPTools ~/.julia/packages/POMDPTools/7Rekv/src/POMDPDistributions/sparse_cat.jl:26
  SparseCat(::Any, ::AbstractArray)
   @ POMDPTools ~/.julia/packages/POMDPTools/7Rekv/src/POMDPDistributions/sparse_cat.jl:16
  SparseCat(::V, ::P) where {V, P}
   @ POMDPTools ~/.julia/packages/POMDPTools/7Rekv/src/POMDPDistributions/sparse_cat.jl:11

New:

julia> SparseCat(b)
                SparseCat distribution           
      ┌                                        ┐ 
    1 ┤■■■■■■■■■ 0.1                             
    2 ┤■■■■■■■■■ 0.1                             
    3 ┤■■■■■■■■■■■ 0.123                         
    4 ┤■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 0.4   
    9 ┤■■■■■■■■■■■■■■■ 0.177                     
   10 ┤■■■■ 0.05                                 
   13 ┤■■■■ 0.05                                 
      └                   

julia> SparseCat(b; check_zeros=false)
                SparseCat distribution           
      ┌                                        ┐ 
    1 ┤■■■■■■■■■ 0.1                             
    2 ┤■■■■■■■■■ 0.1                             
    3 ┤■■■■■■■■■■■ 0.123                         
    4 ┤■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■■ 0.4   
    50                                        
    60                                        
    70                                        
    80                                        
    9 ┤■■■■■■■■■■■■■■■ 0.177                     
   10 ┤■■■■ 0.05                                 
   110                                        
   120                                        
   13 ┤■■■■ 0.05

Other changes in the files is just removing whitespace.

@zsunberg
Copy link
Member

zsunberg commented Dec 4, 2023

@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 zsunberg merged commit 4aca91a into master Dec 4, 2023
8 checks passed
@dylan-asmar dylan-asmar deleted the DiscreteBelief_display branch December 11, 2023 20:16
@dylan-asmar
Copy link
Member Author

@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?

@zsunberg
Copy link
Member

@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.

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.

2 participants