dollabs / pamela

Probabalistic Advanced Modeling and Execution Learning Architecture
Apache License 2.0
233 stars 13 forks source link

Fix using default bounds for TPN's generated with HTN method #114

Closed tmarble closed 7 years ago

tmarble commented 7 years ago

Consistently throw exceptions in HTN generation using fatal-error Closes #98 Closes #108

Signed-off-by: Tom Marble tmarble@info9.net

pmdoll commented 7 years ago

In test/pamela/HTN/coverage.main.tpn.edn, line 225 changed from #{:costle-87 :tc-86 :rewardge-88} to #{:rewardge-89 :costle-88 :tc-87} . What caused the sort order to change? For consistency, I think edn should be sorted just before writing to the file. Is it not the case?

tmarble commented 7 years ago

Clearly the UID's are offset by one as there was probably a new constraint which was created since the previous rubric.

The EDN is sorted insomuch as the keys of maps are in order. However the values are not sorted. In particular sets do not have an order. If we start to have failures due simply to this class of problems we can re-examine the merits of doing literal file contents comparison.

pmdoll commented 7 years ago

Motivation behind literal file content comparison is to ensure and verify that code changes do not inadvertently introduce an unexpected change in produced artifacts which could have ripple effects.

For this PR, I do expect minor changes in UIDs but was wondering what caused the set values to be printed in different order than before. So it seems like it is definitely a change in code that caused the insertion order to be different.

dcerys commented 7 years ago

As mentioned above, sets, by default are unsorted. If the order was consistent beforehand, we were just lucky. Although we could address this by using sorted-set in cases like this, that seems like a slippery slope of code obfuscation. If necessary, I'd rather have the equivalence checker do the work.

tmarble commented 7 years ago

For reference:

pamela.cli> (= {:a 123 :b #{:third :first :second}} {:b #{:first :second :third} :a 123})
true
pamela.cli> (= "{:a 123 :b #{:third :first :second}}" "{:b #{:first :second :third} :a 123}")
false
pamela.cli> 

Our current checks are equivalent to the second kind of comparison.

dcerys commented 7 years ago

Since it doesn't fully close #98, I'll reopen it after closing this PR