Migrating to EzXML; adding pretty-printing #23

jmuchovej · 2023-06-06T02:57:40Z

Migrating from LightXML to EzXML. Enabling states/actions/observations to mapped onto readable names rather than numeric-oriented values.

In a similar vein to JuliaPOMDP/POMDPFiles.jl#13, but for the XML format (so that it's possible to more quickly inspect the XML, if necessary.

I ended up rewriting the reader as well to fully remove the LightXML requirement which seems require earlier versions of Julia and hasn't been updated in >2 years.

One thing to node is that I haven't figured out how to set the XML encoding to ISO 8859-1 instead of UTF-8. From this SO answer, it seems that UTF-8 is more general, thus more permissive in characters, but I'm not sure whether POMDPXFiles.jl should guard against non-ASCII characters or if it should just be a warning to folks who need to directly manipulate .pompdx files?

Migrating from LightXML to EzXML. Enabling states/actions/observations to mapped onto readable names rather than numeric-oriented values.

Cleaning up imports from the stand-alone scripts I used to prototype. Also checking that tests pass.

zsunberg · 2023-06-06T03:54:52Z

src/reader.jl

+These are `0-indexed`, so `0` maps to the first action.
+"""
+
+function read_pomdp(filename::String)


This is an exceedingly strange name for a function that reads in alpha vectors. I think we could name it something better and deprecate read_pomdp.

I was also confused by its name. 😅

Perhaps read_policy?

Then read_pomdp (at some time TBD) would go from .pomdpx to POMDP? Though I’m not sure the utility of this.

I like read_policy, but let's also have a deprecation for any downstream packages to transition smoothly.

zsunberg · 2023-06-06T03:58:46Z

Wow, thanks @jmuchovej this is quite an extensive overhaul. I am a little nervous about such a big change so it will probably take me a few days to think about it.

Also, wow this package is from the good old days... there are a lot of fascinating relics.

zsunberg · 2023-06-06T04:11:32Z

It would be easier to trust big PRs like this if we had coverage set up, but I do not know how to do that.

jmuchovej · 2023-06-06T14:02:17Z

Agreed. 😅

I'm not sure what coverage would look like for something like this. I know that the tests pass, but there are only three tests.

It might be possible to draw on the POMDPX examples from the JuliaPOMDP/sarsop repo, then do some degree of comparisons there.

I probably won't be able to get to that until later this week or the weekend, though.

Using the Tiger POMDP, I go the expected results – but it was manual inspection, so definitely not as trustworthy as coverage reports.

I'm not sure if it's desirable to have POMDPXFiles.jl rely on POMDPFiles.jl for coverage – but it might be possible to use that to read the *.pomdp files, then dump them to *.pomdpx to verify that the conversion from POMDP → *.pomdpx is what's expected.

I'm not sure about the reading of policies, though. Presumably that would just warrant more examples than just the Tiger POMDP. (Which should be "trivial" to add, given other POMDPX files and using SARSOP (instead of NativeSARSOP).)

codecov · 2023-06-17T00:44:32Z

Codecov Report

Patch coverage: 84.04% and project coverage change: -2.93 ⚠️

Comparison is base (bcc061b) 83.02% compared to head (c89cd65) 80.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
- Coverage   83.02%   80.09%   -2.93%     
==========================================
  Files           4        4              
  Lines         271      206      -65     
==========================================
- Hits          225      165      -60     
+ Misses         46       41       -5

Impacted Files	Coverage Δ
src/POMDPXFiles.jl	`100.00% <ø> (ø)`
src/writer.jl	`82.95% <82.84%> (-11.86%)`	⬇️
src/reader.jl	`94.73% <94.73%> (ø)`

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

zsunberg

@jmuchovej thanks! I took a look at this and made a few minor suggestions.

More importantly, I activated codecov, which indicates we need a pretty=true test.

Even more importantly, I tested SARSOP with the new version of POMDPX and it failed. I am guessing that this is due to handling of terminal states. We really should add a test for terminal states in this package, but I am not sure what the best way to do that is. In order to merge this package, we need some kind of test including terminal states and either manual verification that SARSOP will work with this change, or integration testing with SARSOP by including SARSOP as a test dependency of this package.

zsunberg · 2023-06-16T00:14:39Z

src/reader.jl

+These are `0-indexed`, so `0` maps to the first action.
+"""
+
+function read_pomdp(filename::String)


I like read_policy, but let's also have a deprecation for any downstream packages to transition smoothly.

zsunberg · 2023-06-16T02:56:26Z

src/writer.jl

+    next!(pbar)
+
+    if px.pretty
+        all_states = join(["s_$(px.s_name(s))" for s=ordered_states(p)], " ")


Can we put the "s_" prepend in the default for px.s_name so that the user can override it if they want to?

Is there some reason all states need to start with s?

So, my main motive for this was that I’m not certain that a state tiger-left and observation tiger-left won’t be confused by the POMDPX parser (from upstream sarsop). If you know whether this would be the case, then I’m happy to remove it, period.

I suppose I can also test that, too. 😂 I was just focused on other things when generating the XML. Additionally, I’ve noticed that there’s some bug that leads to repeating XML nodes, so I need to clean that up before testing whether the POMDPX parser can handle same-name states/actions/observations.

I think it is a good idea to have the "s_" by default. I just think it should be implemented like this in the POMDPXFile type:

s_name::Function = s->"s_"*normalize(s)

that way, people can override it if they want.

zsunberg · 2023-06-17T15:42:54Z

src/writer.jl


-    initial_belief::Vector{Float64}
+    s_name::Function = normalize


One other thing that I just thought about s_name is a field with an abstract type, which can cause performance issues: https://docs.julialang.org/en/v1/manual/performance-tips/#Avoid-fields-with-abstract-type (and writing POMDPXFiles is sometimes the slowest part of solving a problem with SARSOP.jl, so performance will be important. I suggest using the pattern suggested in that link.

zsunberg · 2023-06-17T15:43:28Z

src/writer.jl

+POMDPXFile(filename::String, pretty::Bool) = POMDPXFile(; filename=filename, pretty=pretty)
+POMDPXFile(filename::String) = POMDPXFile(filename, false)


you can do this in one line

Suggested change

POMDPXFile(filename::String, pretty::Bool) = POMDPXFile(; filename=filename, pretty=pretty)

POMDPXFile(filename::String) = POMDPXFile(filename, false)

POMDPXFile(filename::String, pretty::Bool=true) = POMDPXFile(; filename=filename, pretty=pretty)

jmuchovej added 2 commits June 5, 2023 22:16

Migrating to EzXML; adding pretty-printing

041b5fc

Migrating from LightXML to EzXML. Enabling states/actions/observations to mapped onto readable names rather than numeric-oriented values.

Cleaning up imports; checking tests

2138d8f

Cleaning up imports from the stand-alone scripts I used to prototype. Also checking that tests pass.

zsunberg reviewed Jun 6, 2023

View reviewed changes

jmuchovej added 5 commits June 6, 2023 12:30

Reorganize pretty printing; force use Base.string

68dce8f

Bringing impl closer to that of POMDPFiles

d6891fa

Typos in var_name access.

f6a4dfb

Forgot to remove [s|a|o]name params in build_! funcs

77b9dd5

EzXML wasn't imported correctly?

2075bcb

jmuchovej force-pushed the add-pretty-printing branch from 3f7a573 to c16d0e4 Compare June 9, 2023 16:30

Forgot to -1 on indices b/c C++ starts at 0.

63d063d

jmuchovej force-pushed the add-pretty-printing branch from c16d0e4 to 63d063d Compare June 9, 2023 16:34

jmuchovej and others added 2 commits June 9, 2023 12:37

Add fallback values for unspecified T/O/R

22e8b52

Merge branch 'master' into add-pretty-printing

c89cd65

zsunberg requested changes Jun 17, 2023

View reviewed changes

zsunberg reviewed Jun 17, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrating to EzXML; adding pretty-printing #23

Migrating to EzXML; adding pretty-printing #23

Uh oh!

jmuchovej commented Jun 6, 2023 •

edited

Loading

Uh oh!

zsunberg Jun 6, 2023

Uh oh!

jmuchovej Jun 6, 2023

Uh oh!

zsunberg Jun 16, 2023

Uh oh!

zsunberg commented Jun 6, 2023

Uh oh!

zsunberg commented Jun 6, 2023

Uh oh!

jmuchovej commented Jun 6, 2023 •

edited

Loading

Uh oh!

codecov bot commented Jun 17, 2023

Uh oh!

zsunberg left a comment

Uh oh!

zsunberg Jun 16, 2023

Uh oh!

zsunberg Jun 16, 2023

Uh oh!

zsunberg Jun 17, 2023

Uh oh!

jmuchovej Jun 17, 2023

Uh oh!

zsunberg Jun 17, 2023

Uh oh!

zsunberg Jun 17, 2023

Uh oh!

zsunberg Jun 17, 2023

Uh oh!

Uh oh!

		POMDPXFile(filename::String, pretty::Bool) = POMDPXFile(; filename=filename, pretty=pretty)
		POMDPXFile(filename::String) = POMDPXFile(filename, false)

Migrating to EzXML; adding pretty-printing #23

Are you sure you want to change the base?

Migrating to EzXML; adding pretty-printing #23

Uh oh!

Conversation

jmuchovej commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsunberg commented Jun 6, 2023

Uh oh!

zsunberg commented Jun 6, 2023

Uh oh!

jmuchovej commented Jun 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 17, 2023

Codecov Report

Uh oh!

zsunberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmuchovej commented Jun 6, 2023 •

edited

Loading

jmuchovej commented Jun 6, 2023 •

edited

Loading