Skip to content

Formula type refactoring #251

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

Merged
merged 8 commits into from
Feb 13, 2025

Conversation

nimec01
Copy link
Collaborator

@nimec01 nimec01 commented Jan 31, 2025

Closes #248

This also includes the change from "literals" to "atomics" at the Table type (there might be an overlap with #239 here).
Some tasks need to be adapted on the Autotool-side after this gets merged.

One thing I'm not sure about is the usage of atomics here: https://github.com/nimec01/logic-tasks/blob/e57f1cd58cbc39c649fd50618ebbc192997be86b/src/Formula/Resolution.hs#L50-L57

There also might be more renaming to consider since there are still name overlaps in UniversalParser, Formula.Types (and potentially Trees.Types).

Example: Neg-Constructor of Literal and Neg-Constructor of Neg (at UniversalParser)

@jvoigtlaender
Copy link
Member

The merge of master above includes #239, so maybe this part of the initial comment above is now obsolete:

there might be an overlap with #239 here

@nimec01 nimec01 marked this pull request as ready for review February 11, 2025 12:10
@nimec01
Copy link
Collaborator Author

nimec01 commented Feb 11, 2025

Example: Neg-Constructor of Literal and Neg-Constructor of Neg (at UniversalParser)

This exact case could also be resolved by changing the new constructor names to:

data Literal
    = Positive { letter :: Char} -- ^ positive sign
    | Negative { letter :: Char} -- ^ negative sign

instead of

data Literal
    = Pos { letter :: Char} -- ^ positive sign
    | Neg { letter :: Char} -- ^ negative sign

@jvoigtlaender
Copy link
Member

Does this also still warrant some action here?

-- | Return the used char in an atom. Can be removed after #248 was merged.
atomicChar :: Literal -> Char
atomicChar (Literal c) = c
atomicChar _ = error "this should not happen"

@jvoigtlaender
Copy link
Member

And yes, Positive and Negative seems fine.

@jvoigtlaender
Copy link
Member

About this part:

One thing I'm not sure about is the usage of atomics here: https://github.com/nimec01/logic-tasks/blob/e57f1cd58cbc39c649fd50618ebbc192997be86b/src/Formula/Resolution.hs#L50-L57

Where does the uncertainty come from? That piece of code seems to be semantically equivalent to what it was before. Do you suspect there was a semantic error so far?

@jvoigtlaender
Copy link
Member

What appears from looking through the code here is also that there seem to be further cases in the code where "literal" vs. "atom" is confused, not caught by #239 (maybe because the code search there was for "literal" rather than also possible variations of it).

For example, in here:

partialGrade' :: OutputCapable m => MinInst -> Dnf -> LangM m
partialGrade' MinInst{..} sol = Max.partialMinMax corLits dnf sol allMinTerms False
where
corLits = atomics dnf
allMinTerms = not $ all (\c -> amount c == length corLits) $ getConjunctions sol
I guess corLits should be correctAtoms.

And here:

genForNF :: (Int,Int) -> (Int,Int) -> [Char] -> Gen (Int, [Char])
genForNF (minNum,maxNum) (minLen,maxLen) lits
| null nLits || invalidLen || invalidNum = pure (0, [])
| otherwise = do
num <- chooseInt (minNum, min maxNum upperBound)
pure (num, nLits)
where
nLits = nub lits
invalidLen = minLen <= 0 || minLen > maxLen || minLen > length nLits
invalidNum = minNum <= 0 || minNum > maxNum || minNum > upperBound
upperBound = lengthBound (length nLits) maxLen
(and also in uses of this function) the "lits" are really "atoms"?

@nimec01
Copy link
Collaborator Author

nimec01 commented Feb 13, 2025

About this part:

One thing I'm not sure about is the usage of atomics here: https://github.com/nimec01/logic-tasks/blob/e57f1cd58cbc39c649fd50618ebbc192997be86b/src/Formula/Resolution.hs#L50-L57

Where does the uncertainty come from? That piece of code seems to be semantically equivalent to what it was before. Do you suspect there was a semantic error so far?

I was wondering if a simple literals call would work here. Thelits variable could then of course include elements using the Negative constructor, but would this be a problem? (I have not really looked into the code behind the Resolution task)

@nimec01
Copy link
Collaborator Author

nimec01 commented Feb 13, 2025

What appears from looking through the code here is also that there seem to be further cases in the code where "literal" vs. "atom" is confused, not caught by #239 (maybe because the code search there was for "literal" rather than also possible variations of it).

For example, in here:

partialGrade' :: OutputCapable m => MinInst -> Dnf -> LangM m
partialGrade' MinInst{..} sol = Max.partialMinMax corLits dnf sol allMinTerms False
where
corLits = atomics dnf
allMinTerms = not $ all (\c -> amount c == length corLits) $ getConjunctions sol

I guess corLits should be correctAtoms.
And here:

genForNF :: (Int,Int) -> (Int,Int) -> [Char] -> Gen (Int, [Char])
genForNF (minNum,maxNum) (minLen,maxLen) lits
| null nLits || invalidLen || invalidNum = pure (0, [])
| otherwise = do
num <- chooseInt (minNum, min maxNum upperBound)
pure (num, nLits)
where
nLits = nub lits
invalidLen = minLen <= 0 || minLen > maxLen || minLen > length nLits
invalidNum = minNum <= 0 || minNum > maxNum || minNum > upperBound
upperBound = lengthBound (length nLits) maxLen

(and also in uses of this function) the "lits" are really "atoms"?

I will address this in a separate pull request.

@jvoigtlaender
Copy link
Member

Thelits variable could then of course include elements using the Negative constructor, but would this be a problem? (I have not really looked into the code behind the Resolution task)

I'll merge this PR here, then open a separate issue related to that point. There is actually a subtlety about the implementation of resolution I was not aware of before.

@jvoigtlaender jvoigtlaender merged commit 7ea5ef8 into fmidue:master Feb 13, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some refactoring in formula types
2 participants