Allow restricting positive literals in formula besides percentTrueEntries - #353
Allow restricting positive literals in formula besides percentTrueEntries#353JNAOB wants to merge 14 commits into
percentTrueEntries#353Conversation
Ich bin mir ziemlich sicher, dass das der Fall ist. Also könnte diese Löschung im PR schon vorgenommen werden. |
Ob das ein mittlerweile obsoletes Artefakt ist, vermag ich nicht mit Sicherheit zu sagen. Vorsichtshalber denke ich: behalten, nur für trueEntries. (Wobei: geht es da wirklich um PickSpec, oder um einen Config-Check für Pick außerhalb der Testsuite?) |
|
I'm a bit worried whether this change:
is really without adverse consequences. The code so far might at any given point where logic-tasks/flex/resStepFlex.flex Line 67 in e54dd27 isSubsequenceOf function is sensitive to duplicates. Hence, the outcome of that check could be affected. To prevent that, one might defensively apply nubOrd:
not $ nubOrd (literals (snd solution)) `isSubsequenceOf` nubOrd (literals clause1)or even not $ nubSort (literals (snd solution)) `isSubsequenceOf` nubSort (literals clause1)in order to defend against the case that maybe the current code even assumes that One might argue that |
|
Hierzu:
würde ich erstmal sagen, lassen wir die Testabdeckung so wie sie im Moment ist. Also wenn dieser Aspekt bisher nicht epxlizit getestet wurde, muss das jetzt auch nicht explizit hinzu (außer es ist im PR eh schon enthalten). Und hierzu:
können Sie dann einfach die passende Konsequenz aus obiger Entscheidung ziehen und entweder noch umbenennen oder eben nicht. |
Das kann durchaus sein. Die Entscheidung kam aus diesem Kommentar #274 (comment), kann aber auch wieder geändert werden. Ich habe versucht, überall wo es wichtig ist nubOrd hinzuzufügen. In dem flex Beispiel ist es denke ich nicht nötig, da Klauseln sowieso keine Duplikate enthalten. Da jetzt aber auch viel Zeit vergangen ist, in der andere PRs neue Nutzungen von |
|
Die API-Änderung ist schon okay, also wie in dem verlinkten Kommentar. Dass Sie daran bei den Änderungen schon gedacht hatten, beruhigt mich. Wobei wie gesagt mir nicht klar ist, ob |
|
Es wurden nur die Implementierung von Mir stellt sich jetzt die Frage, ob wir es uns leisten können in den drei veränderten Implementierungen provisorisch Dann müsste nicht jede Verwendung überprüft werden und die API bleibt einheitlicher und näher an der Vorgängerversion. |
|
Ja, finde ich sinnvoll. Wobei etwa an der entsprechenden Stelle gar nicht |
|
Jetzt sind wir also an diesem Punkt:
Irgendwas noch umbenennen oder nicht? |
|
|
||
| instance Formula Cnf where | ||
| literals (Cnf set) = Set.toList $ Set.unions $ Set.map (Set.fromList . literals) set | ||
| literals (Cnf set) = sort $ concatMap literals set |
There was a problem hiding this comment.
Das ginge, hier und an ähnlichen Stellen, wahrscheinlich auch effizienter mit wiederholtem merge.
| tabulate "all literals" (map show $ literals cnf) $ | ||
| tabulate "positive literals" (map show $ filter isPositive $ literals cnf) $ | ||
| tabulate "negative literals" (map show $ filter (not . isPositive) $ literals cnf) $ | ||
| let uniqueLiterals = nubOrd $ literals cnf in |
There was a problem hiding this comment.
Immer wenn jetzt nubOrd auf einer bereits als sortiert bekannten Liste läuft, hätte etwas wie „map head . group“ bessere Komplexität.
2 participants
CLOSE #219
Breaking Changes:
literalsfürCnf,Dnfund Syntaxbäume enthält jetzt DuplikatepercentTrueEntriesheißt jetztpercentRangeModeund ist vom TypPercentRangeModewithRatiowird in diesem Repo nicht mehr genutzt und ist jetzt deprecated. Wenn es nicht außerhalb des Repos benutzt wird, kann es auch gelöscht werden.Module für die
posLiteralsgetestet wirdDecideFillSpecPickSpechat noch einen eigenen Config Check, dass die Range mindestens 30 betragen soll. Ist das ein Artefakt aus der Zeit in dercheckTruthValueRangenoch nicht darauf geachtet hat, dass wirklich eine Instanz möglich ist? Also kannPickan die anderen beiden angepasst werden, in dem der 30 % check gelöscht wird oder soll der Check in Pick auch aufposLiteralsangewendet werden oder weiterhin nur auftrueEntries?In
MinMaxwar der Test eh nie eingeschränkt.Kann es sein, dass im Allgemeinen die Test Suite sowieso nie
percentTrueEntriesgetestet hat, da abgesehen vonMinMaxalle anderen Module nur auf Syntaxbäumen getestet werden und dortpercentTrueEntriesverboten ist?Da in Syntaxbäumen
percentTrueEntriesverboten ist, entspricht das doch quasi keinem Test. Fürs Erste wurde das Verbot auch fürpercentRangeModeübernommen.Wenn diese Fragen geklärt sind, können die Namen und Ausgaben der Funktionen
checkFullRangeForSynTrees,checkTruthValueRangeAndFormulaConfusw. angepasst werden.