Add "pretty printing"

[jmuchovej]

The POMDP file spec supports enum-like representation of states/actions/observations.

Would this be an acceptable PR? (I did it for myself, but figured I'd add it upstream if y'all wanted it.)

Essentially, there's a "pretty printing" mode that uses the string representation of each states/actions/observations and uses those to generate a POMDP file. It does take the longer-form setup (1 line per pairing, vs. the matrix setup), but it's straightforward to support the matrices in the "pretty printing" mode.

I did this before #10 was accepted, so I'll need to rebase, but figured I'd open the PR before rebasing.

[mykelk]

Any opinions on this @zsunberg ? Does it make sense for it to be in POMDPFiles? Or somewhere else?

[zsunberg]

I think it is a good idea to have here since it is a valid POMDP file format according to
https://pomdp.org/code/pomdp-file-spec.html

[mykelk]

Cool. Let's do it.

[zsunberg]

@jmuchovej make sure to mark this PR as not a work in progress when it is ready.

In the long term, having a keyword argument for Base.write(::IO, ::POMDP) is definitely not the right way to give users access to the option. In fact, I don't think Base.write is appropriate for this at all (#14 ), but since that would require a refactor with some design decisions, I'm happy to accept this PR with the keyword argument.

Ideally, someday, we would also be able to read files written with state names in and preserve the names in the POMDP representation, but that is probably for another day.

[jmuchovej]

Ooh. I see.

Aside: it seems that POMDPFiles exports POMDPFile (which is presumably a struct), but I couldn't find a definition for it?

If it were [re]defined, then perhaps that could be used instead of a keyword argument? (Though this seems unwieldy, it could something like...

Base.write(io::IO, pomdp::POMDP) = ...
Base.write(io::IO, pomdpfile::POMDPFile) = ...

So then it retains backwards compatibility, but sidesteps the need for the keyword argument.)

Additionally, I just did some stuff with EzXML and they export a prettyprint([io::IO], ...) function. So perhaps, that could be a suitable alternative to in line with your suggestion in #14, maybe Base.print is for the native (numeric) representation and prettyprint would be for this representation?

[zsunberg]

I don't think we need backwards compatibility since only one or two packages depend on this and semver makes it so that upgrading isn't terrible.

I actually think that we should just copy read_pomdp and have a function called write_pomdp rather than Base.write. That can have keyword arguments. There is no need to be fancy.

[zsunberg]

Note that the CI tests are not passing, please take a look and see if you can fix it.

[jmuchovej]

So, the CI is failing because #10 introduced POMDPModels as a direct dependency, rather than a test dependency. (I removed it as a direct dependency to more closely match the original repo I cloned, pre #10.)

I know how to fix it, but I'm not sure where to put POMDPModels.

[zsunberg]

POMDPModels is now a direct dependency because the package uses TabularPOMDP.

We should use using POMDPModels: TabularPOMDP in the main module file to make this clear.

[jmuchovej]

Oh! I see now. Completely missed that in src/read.jl. 😅 I moved it to the main module file.

[codecov]

Codecov Report

Patch coverage: 85.14% and project coverage change: +3.81 🎉

Comparison is base (4766f6e) 61.78% compared to head (6d1e1f6) 65.60%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #13      +/-   ##
==========================================
+ Coverage   61.78%   65.60%   +3.81%     
==========================================
  Files           3        3              
  Lines         280      314      +34     
==========================================
+ Hits          173      206      +33     
- Misses        107      108       +1     

Flag	Coverage Δ
unittests	65.60% <85.14%> (+3.81%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/POMDPFiles.jl	100.00% <ø> (ø)
src/reader.jl	56.27% <66.66%> (ø)
src/writer.jl	85.71% <85.71%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[zsunberg]

Thanks for the contribution!

This will need a bit more work before merging. See if you can fix the comments below. Then we can talk a bit more about what to name the functions.

[jmuchovej]

I did some premature refactoring. print contains the original implementation that opts for a numeric representation, prettyprint is self-explanatory (I hope 😅).

They aren't exported at the moment, but I imagine they probably need to be.

[zsunberg]

@jmuchovej , thanks again for your contribution! This looks great! Would you be willing to make a test for this? Perhaps along the lines of

struct TestPOMDP <: POMDP{Int, Int, Int} end
POMDPs.states(::TestPOMDP) = (1,)
POMDPs.transition(::TestPOMDP) = Deterministic(1)
# etc.

@test sprint(POMDPFiles.write, TestPOMDP()) == """
<file contents>
"""

If you don't have time for that, no worries - I didn't ask you to do this up front. I will probably accept the PR anyways.

I think for function naming we should follow the convention of CSV.jl and have POMDPFiles.read and POMDPFiles.write that are differ. If you are willing to make that change, great! If not, I can do it. POMDPFiles.prettyprint would be ok too if you want to have a separate function rather than just an option.

[zsunberg]

(I just pushed changes turning all the tabs into 4 spaces)

[jmuchovej]

I think for function naming we should follow the convention of CSV.jl and have POMDPFiles.read and POMDPFiles.write that are differ.

Should we follow CSV.jl over something like FileIO.jl? (I guess maybe not ‘cause CSV.jl is more highly starred than any singular FileIO.jl package?)

Would you be willing to make a test for this?

Definitely! I might not get to this until later today or the weekend, though.

Also, if the functions are changing, what other packages need to be updated? (I can look into this when writing tests, I just figured you might know offhand.)

[zsunberg]

Oh, I did not know about FileIO! After an initial glance it seems like a good idea. I don't know enough to have a strong vote between FileIO and CSV, but my initial reaction is that FileIO might be better.

[jmuchovej]

FileIO.jl can also let use do things like dispatch on file extensions (which I think is cool just for kicks). Point being, it might be feasible to unify POMDPXFiles and POMDPFiles since they're fairly closely tied to begin with (though definitely not necessary right now).

[jmuchovej]

Since we're going to go for the FileIO interface, what do you think of skipping the transition this PR and just having it focus on adding "pretty printing" (and associated tests)?

I'm down to the FileIO transition, but I think doing that here will be half-hearted 'cause the FileIO interface should also apply to all the read-like functions as well.

[zsunberg]

I like the way you're thinking, @jmuchovej !! I'm fine with accepting this with just the pretty printing and doing FileIO in another PR.

I will merge this once the tests pass.

[jmuchovej]

Ok! I think these are all good to go now (tests and all).
I'll start on the FileIO.jl bits once this is merged.

Also, I figure, since this is gonna be a short-lived series of functions, maybe there's no need for a patch bump? (Since the next one is gonna be a minor version bump anyway (afaik, Julia considers <1 minor bumps to be breaking).)

[zsunberg]

Wow @jmuchovej , this has turned into quite a beautiful PR. Thank you for making such a high-quality contribution to this package!

Also, I figure, since this is gonna be a short-lived series of functions, maybe there's no need for a patch bump? (Since the next one is gonna be a minor version bump anyway (afaik, Julia considers <1 minor bumps to be breaking).)

Yes, I was just going to wait for FileIO to tag a new version. But if it is going to take you a while, then let me know and I can tag a version with just this.