-
Notifications
You must be signed in to change notification settings - Fork 5
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
Adding functions to get all classes and all values #13
Conversation
@jkoppel This looks like a useful addition! Could you please formulate some laws for the new operation and add them as quickcheck tests to the testsuite? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good addition. Needs laws and tests.
classes Equiv {entries = mref} = do | ||
allEntries <- Map.elems <$> readSTRef mref | ||
rootEntries <- filterM isRoot allEntries | ||
mapM (fmap Class . newSTRef) $ rootEntries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that a law could be that classes e
should be the same set as map (getClass e) (values e)
. However, since newSTRef
is involved here, the exact meaning of the same set needs to be figured out.
Woah! 8 months later, and I had already forgotten this PR exists. This is demanding a lot from me because I've forgotten so much about how this library works. Law for values: values_equiv:
(Set.fromList <$> values) `Set.union` Set.fromList [x,y] === equate x y >> (Set.fromList <$> values) There are similar laws for the interaction between values/remove and values/combine. I do not know how to write these because the API lacks a way to get the values for a class. (Also, does getClass add a new value to the monad? That's not what I expect from anything with 'get' in the name. Does anything else do this?) Overall, the QuickCheck tests are not set up to make it easy to create an arbitrary state, which makes it difficult to do this properly. The ideal in property-based testing is that the properties being tested, if formally proven, would combine to imply full correctness of the system. The existing tests only inspect a few specific families of state, and I don't believe the other properties under test are sufficient to show these are equivalent to all other states. Since the existing standard of tests is less than this golden one, I have no idea what you/pa-ba would deem an acceptable level of testing. E.g.: I started by writing the following QuickCheck test: prop_values l = runInt $ do
mapM (\x -> equate x x) l
vs <- values
return (Set.fromList vs == Set.fromList l) However, this is far weaker than the values_equiv property I wrote above. I do not remember how STRefs are used for classes and thus cannot easily think about the equality questions at the moment. Beyond me simply not remembering many details about how this library works, I cannot fulfill this request because I do not know the standard of testing demanded by this codebase. |
@jkoppel: You got a point here. The tests are not that deep. In particular, they do not set up a random initial state, meaning a mapping from some set of values to their equivalence class. They will always start with the empty state (no values, no classes). I do not ask for a significant improvement on the testsuite. I think it is sufficient to add some tests in the spirit of the testsuite that catch elementary bugs. I think these two tests would be enough:
|
@andreasabel Looks like I forgot about this again in the rush for the ICFP deadline. Now we've just handed in our camera-ready, and this PR is holding us up from being able to release on Hackage, as our package relies on my fork with this feature. I have added both tests as requested. Let's get this merged! |
combine x y = lift $ combine x y | ||
values = lift values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new methods should also have a default
signature. Fixing this myself...
Adding the
values
andclasses
methods to get all values and classes encountered.Also addresses #7.
Example uses: