Switch some lists to Set from containers #3761

NoahCardoso · 2024-05-30T19:42:26Z

Nub is slow and suggestion 209 pops up a lot. When working with lists that have Ord we can use toList and fromList from Data.Set. Nub has quadratic complexity because it checks each element of the list against every other element of the list. If you convert your list to a Set, you can avoid this quadratic complexity because Set does not allow duplicate elements.

JacquesCarette · 2024-05-31T15:50:29Z

code/drasil-docLang/lib/Drasil/DocumentLanguage.hs

@@ -434,7 +435,7 @@ mkTraceabilitySec (TraceabilityProg progs) si@SI{_sys = sys} = TG.traceMGF trace

 -- | Helper to get headers of rows and columns
 header :: ([UID] -> [UID]) -> SystemInformation -> [Sentence]
-header f = TM.traceMHeader (f . nub . Map.keys . (^. refbyTable))


This seems odd: I don't think Map can have duplicate keys, so it seems to be that this nub is entirely redundant!

JacquesCarette · 2024-05-31T15:51:10Z

code/drasil-gool/lib/GOOL/Drasil/State.hs

@@ -468,7 +469,7 @@ callMapTransClosure = over callMap tClosure
        traceCalls :: Map QualifiedName [QualifiedName] -> [QualifiedName] -> 
          [QualifiedName]
        traceCalls _ [] = []
-        traceCalls cm (c:cs) = nub $ c : traceCalls cm (nub $ cs ++ 
+        traceCalls cm (c:cs) = toList . fromList $ c : traceCalls cm (toList . fromList $ cs ++ 


We shouldn't have had two calls to nub - may as well do this only once.

JacquesCarette · 2024-05-31T15:52:11Z

code/drasil-lang/lib/Language/Drasil/CodeExpr/Extract.hs

@@ -90,7 +91,7 @@ eNamesRI' (UpFrom il)     = eNames' (snd il)

 -- | Get dependencies from an equation.
 eDep :: CodeExpr -> [UID]
-eDep = nub . eNames
+eDep = toList . fromList . eNames


we should have our own 'utility' function that does exactly this, instead of inlining toList . fromList everywhere (thus making the code obscure).

I'm fairly certain that toList . fromList would also sort the list, since Haskell's internal Set is really an ordered binary tree (like we discussed yesterday, @NoahCardoso). That is to say, I'm a bit skeptical that eDep uses care for the order of the UIDs if nothing changed in stable/ (which is a good thing). Have you tried switching the type here from a list to a set instead? Our eDep-like (UID-dependency searching) tools have a lot of nub usage, so switching to a set type might make sense.

@balacij should I change every list of UIDs to Set of UIDs?

I think there will be many, but I'm not sure if all should be changed. For example, if we have a list of UIDs in a chunk, we will need to check if order matters/makes sense.

Should chunks should be kept as a list but everything else should be set when possible?

It's likely a lot more subtle than that: the question is when does order matter, and when it doesn't. And sometimes order matters only for display purposes.

We do over-use lists, but that's quite typical of most Haskell projects which make lists too easy to use!

balacij · 2024-06-06T17:24:17Z

code/drasil-lang/lib/Language/Drasil/Expr/Extract.hs

@@ -76,4 +76,4 @@ eNamesRI' (UpFrom il)     = eNames' (snd il)

 -- | Get dependencies from an equation.  
 eDep :: Expr -> [UID]
-eDep = nub . eNames
+eDep = toList . fromList . eNames


@NoahCardoso If you're struggling with figuring out which things should be Sets and which should be lists, I think there are two ways you can go about this:

Research first.

Make a hypothesis and test it.

In other words,

Traverse over all things that use eDep, and then all things that use those things, and figure out if order is somehow important and why it is (or why it shouldn't be...). Thankfully, there isn't too much usage of eDep, this might be easier and better, logically.

Semi-confidently assume that this function should return a Set rather than a list. Our hypothesis of sorts -- the comment just says "Get dependencies from an equation," but it doesn't mention any sort of ordering of the dependencies, it just lists what a specific expression depends on (i.e., variables and functions). You can try this, and see what breaks.

Similar to eDep, I would imagine this extends to sdep, shortdep, eNames, but you can figure it out as you go along.

I took a quick peek at `eDep` usage. It doesn't like there's too much to traverse

λ ~/Programming/Drasil/ glassbrIntro rg "[^a-z]eDep" -ths code/drasil-code/lib/Language/Drasil/Chunk/CodeBase.hs 20:codevars e m = map (varResolve m) $ eDep e 24:codevars' e m = map (varResolve m) $ nub $ eDep' e code/drasil-lang/lib/Language/Drasil/Expr/Development.hs 9: , eDep, eNames, eNames', eNamesRI 15:import Language.Drasil.Expr.Extract (eDep, eNames, eNames', eNamesRI) code/drasil-lang/lib/Language/Drasil/CodeExpr/Extract.hs 2: eDep, eDep', code/drasil-lang/lib/Language/Drasil/CodeExpr/Development.hs 11: eDep, eDep', 25:import Language.Drasil.CodeExpr.Extract (eDep, eDep', eNamesRI, eNamesRI')

eDep is only used in codevars. I have eDep as a Set currently. This is roughly the order of the other functions that relate to eDep. eDep -> codevars -> fstvars -> fstdecl -> genFunc -> genModDef, genModFuncs ... -> ... generates things. Once things start to get generated in gool I assume that order matters. Codervars, fstvars and fstdecl might be able to be Sets however codevars is a list of CodeChunks which are not ord

Also meDep is used for printing. Should it be? I thought it was only for getting dependencies and its order shouldn't matter.

Drasil/code/drasil-lang/lib/Language/Drasil/ModelExpr/Extract.hs

Lines 87 to 89 in 864e138

-- | Get dependencies from an equation.

meDep :: ModelExpr -> [UID]

meDep = nub . meNames

balacij · 2024-06-06T17:25:01Z

Since this is a WIP branch now, I changed the status to a Draft PR.

balacij · 2024-06-27T03:12:20Z

code/drasil-gool/lib/GOOL/Drasil/State.hs

@@ -468,7 +469,7 @@ callMapTransClosure = over callMap tClosure
        traceCalls :: Map QualifiedName [QualifiedName] -> [QualifiedName] -> 
          [QualifiedName]
        traceCalls _ [] = []
-        traceCalls cm (c:cs) = nub $ c : traceCalls cm (nub $ cs ++ 
+        traceCalls cm (c:cs) = toList . fromList $ c : traceCalls cm ( cs ++ 


I think that if you remove the existing toList . fromLists, this PR will be in a mergeable state, no? I think we should do that, since you're now going through the nubd lists noted by stan and figuring out which ones really need a nub and which ones really want to be a different type.

For this example, you can but for nubSort toList . fromList is used to remove all duplicates from the list
nubSort :: Ord a => [a] -> [a]
nubSort = toList . fromList . sort

Yep, we can just revert that one to nub as it was before this PR. This way, we minimize the changes in the PR and put it in an immediately merge-able state with just the few changes from lists to Sets.

After this PR, we can figure out which nubs involve a list that should be a Set or not, and go from there. However, since toList . fromList sorts lists, I think that is also a clue that we can also change this area to using a Set (since a Set in Haskell is ordered already).

NoahCardoso · 2024-07-02T14:57:38Z

The order of eNames does not matter however the order of eNames' (and meDep in drasil-lang/lib/Language/Drasil/ModelExpr/Extract.hs) does in drasil-lang/lib/Language/Drasil/CodeExpr/Extract.hs. I believe the only difference between the two is that the order of eNames' matters for printing. Is this important? I think that unless there is a specific reason that the order is important we should try to replace eNames' with eNames.

JacquesCarette · 2024-07-07T06:27:18Z

code/drasil-code/lib/Language/Drasil/CodeSpec.hs

@@ -192,4 +192,4 @@ getConstraints cm cs = concat $ mapMaybe (\c -> Map.lookup (c ^. uid) cm) cs
 -- | Get a list of 'CodeChunk's from a constraint.
 constraintvars :: ConstraintCE -> ChunkDB -> [CodeChunk]
 constraintvars (Range _ ri) m =
-  map (codeChunk . varResolve m) $ nub $ eNamesRI ri
+  map (codeChunk . varResolve m) $ nub $ toList $ eNamesRI ri


since eNamesRI now returns a Set, isn't the nub redundant?

Yes it has been removed

balacij · 2024-08-21T22:57:06Z

Sorry, why was this closed? Should we keep the branch or delete it? Was there anything we could salvage @NoahCardoso ?

NoahCardoso · 2024-08-22T01:59:21Z

Yes anything of use was adding in #3930

NoahCardoso requested review from JacquesCarette and bmaclach as code owners May 30, 2024 19:42

JacquesCarette requested changes May 31, 2024

View reviewed changes

balacij reviewed Jun 6, 2024

View reviewed changes

balacij marked this pull request as draft June 6, 2024 17:24

balacij mentioned this pull request Jun 7, 2024

Extract duplicate strings into InternalConcepts #3642

Merged

NoahCardoso changed the base branch from master to main June 14, 2024 19:49

balacij requested changes Jun 27, 2024

View reviewed changes

NoahCardoso mentioned this pull request Jun 28, 2024

Drasil Research Group Meeting, Wednesday July 3, 11am, ITB/225 + online #3815

Closed

NoahCardoso mentioned this pull request Jul 5, 2024

Drasil Research Group Meeting, Monday July 8, 3pm, ITB/225 #3834

Closed

JacquesCarette requested changes Jul 7, 2024

View reviewed changes

NoahCardoso force-pushed the fixUsageOfNub branch from 440a2dd to d7d093b Compare July 9, 2024 16:45

NoahCardoso mentioned this pull request Jul 12, 2024

Drasil Research Group Meeting, 3pm, Monday July 15, 2024, ITB/225 #3842

Closed

NoahCardoso force-pushed the fixUsageOfNub branch from d7d093b to 6353236 Compare July 31, 2024 15:08

NoahCardoso added 14 commits July 31, 2024 11:24

Switch some lists to Set from containers

332b7d2

fix import

f19fed6

fix redundancy

5303539

fix redundant use of nub

9d6b220

changed sdep, shortdep and eDep

cd76b51

Changed eNames and getUIDs to Sets

9b2b1b0

rm nub from callMapTransClosure

7f5e492

re added nub

5f46741

change

5bd826c

changed codeExpr eNames to Set

45d229e

rm eDep

9b7e5ac

rm nub in CodeBase.hs

d449dec

small fix

ee1e116

rm unused nub

a86f138

NoahCardoso added 2 commits July 31, 2024 11:24

WIP

cbd1362

undo symbol

2496a8e

NoahCardoso force-pushed the fixUsageOfNub branch from 6353236 to 2496a8e Compare July 31, 2024 15:26

NoahCardoso mentioned this pull request Aug 20, 2024

change some nub to nubOrd #3930

Merged

NoahCardoso closed this Aug 20, 2024

JacquesCarette deleted the fixUsageOfNub branch August 23, 2024 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch some lists to Set from containers #3761

Switch some lists to Set from containers #3761

NoahCardoso commented May 30, 2024

JacquesCarette May 31, 2024

JacquesCarette May 31, 2024

JacquesCarette May 31, 2024

balacij May 31, 2024 •

edited

Loading

NoahCardoso Jun 3, 2024

balacij Jun 3, 2024

NoahCardoso Jun 4, 2024

JacquesCarette Jun 4, 2024

balacij Jun 6, 2024

NoahCardoso Jun 14, 2024

NoahCardoso Jun 17, 2024

balacij commented Jun 6, 2024

balacij Jun 27, 2024

NoahCardoso Jun 27, 2024

balacij Jun 27, 2024

NoahCardoso commented Jul 2, 2024 •

edited

Loading

JacquesCarette Jul 7, 2024

NoahCardoso Jul 8, 2024

balacij commented Aug 21, 2024

NoahCardoso commented Aug 22, 2024

	-- \| Get dependencies from an equation.
	meDep :: ModelExpr -> [UID]
	meDep = nub . meNames

Switch some lists to Set from containers #3761

Switch some lists to Set from containers #3761

Conversation

NoahCardoso commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balacij May 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balacij commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NoahCardoso commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balacij commented Aug 21, 2024

NoahCardoso commented Aug 22, 2024

balacij May 31, 2024 •

edited

Loading

NoahCardoso commented Jul 2, 2024 •

edited

Loading