NonEmpty AdjacencyIntMaps #250

jitwit · 2020-01-22T21:39:09Z

Adding modules for NAIMs

I was having problems in the test suite with reachable about couldn't match type ToVertex AIM with type Int so I commented it out for now.

I changed the complexity comments for vertexCount/edgeCount in AIM, which claimed O(1).

Hopefully not too many names in the documentation are out of sync!

Acked-by: jitwit <[email protected]>

snowleopard · 2020-01-22T23:09:25Z

src/Algebra/Graph/NonEmpty/AdjacencyIntMap.hs

+unsafeNonEmpty :: [Int] -> NonEmpty Int
+unsafeNonEmpty = fromMaybe (error msg) . nonEmpty
+  where
+    msg = "Algebra.Graph.AdjacencyIntMap.unsafeNonEmpty: Graph is empty"


Should be Algebra.Graph.NonEmpty.AdjacencyIntMap. Alternatively, we could move this utility function to Algebra.Graph.Internal to avoid code duplication.

I think Internal sounds good

snowleopard · 2020-01-22T23:13:59Z

src/Algebra/Graph/NonEmpty/AdjacencyIntMap.hs

+-- Complexity: /O(1)/ time and memory.
+--
+-- @
+-- 'AdjacencyIntMap.hasVertex' x (vertex x) == True


I'm a bit confused: do we really need qualifications AdjacencyIntMap. here?

I don't think so. I was following very closely the NonEmpty.AdjacencyMap module

Ha, indeed. If it's redundant, let's drop it there too.

snowleopard · 2020-01-22T23:31:14Z

src/Algebra/Graph/NonEmpty/AdjacencyIntMap.hs

+-- induceJust1 . 'gmap' 'Just'                                    == 'Just'
+-- induceJust1 . 'gmap' (\\x -> if p x then 'Just' x else 'Nothing') == 'induce1' p
+-- @
+-- TODO: double check there's no analogue for AIMs


Yep, I don't think there is one.

snowleopard · 2020-01-22T23:33:35Z

test/Algebra/Graph/Test/NonEmpty/AdjacencyIntMap.hs

+import qualified Data.IntSet                            as IntSet
+
+sizeLimit :: Testable prop => prop -> Property
+sizeLimit = mapSize (min 10)


There is size10 in Algebra.Graph.Test, so this can be dropped.

jitwit · 2020-01-22T23:34:02Z

src/Algebra/Graph/NonEmpty/AdjacencyIntMap.hs

+-- vertexSet . 'clique1'   == Set.'Set.fromList' . 'Data.List.NonEmpty.toList'
+-- @
+vertexSet :: AdjacencyIntMap -> IntSet
+vertexSet = coerce AIM.vertexIntSet


Oops, should be vertexIntSet

(And accompanying documentation functions IntSet.)

Indeed 👍

test/Algebra/Graph/Test/NonEmpty/AdjacencyIntMap.hs

snowleopard · 2020-01-22T23:37:14Z

test/Algebra/Graph/Test/NonEmpty/AdjacencyIntMap.hs

+    test "Axioms of non-empty graphs"   axioms
+    test "Theorems of non-empty graphs" theorems
+
+    putStrLn $ "\n============ Ord NonEmpty.AdjacencyIntMap ============"


It would be nice to reduce code duplication for non-empty tests. We could move some tests into Algebra.Graph.Test.Generic, adding suffix NonEmpty to properties for disambiguation.

testAdjacencyMap would be a model to go off? It seems like most of the functions from the NAM modules are candidates

Yes, I think we can reuse tests for most functions from NonEmpty modules. Just thought that perhaps creating Generic.NonEmpty would be best since then we don't need to do any renaming.

Sorry, I'm not clear about the some of the issues and steps. What would need to be renamed? Does the plan look something like this?

Algebra.Graph.Test.API module should get NonEmpty functions added and the nonempty types should get a nonemptyAPI :: API (G/Mono) (Ord/(~) Int)

also add the ToGraph instance for NAIM

new TestsuiteInt g -> IO () functions go in Algebra.Graph.Test.Generic.NonEmpty

each nonempty module would import the API, Generic, and Generic.NonEmpty modules similar to what Algebra.Graph.Test.AdjacencyMap does

After refreshing my memory on how things are implemented, here is a more concrete plan:

Extend data API g c with new fields that correspond to missing methods (most of them end with the suffix 1). In fact there is a TODO for this:

alga/test/Algebra/Graph/Test/API.hs

Line 58 in 026ba0e

-- TODO: Add missing API entries for Acyclic, NonEmpty and Symmetric graphs.

Add tests for missing methods to Test.Generic or Test.Generic.NonEmpty -- whichever turns out to be clearer/more convenient.

I don't think we need any ToGraph instances for non-empty graphs but perhaps I'm mistaken? Instead, I expect API definitions similar to

alga/test/Algebra/Graph/Test/API.hs

Lines 146 to 148 in 026ba0e

-- | The API of 'AM.AdjacencyMap'.

adjacencyMapAPI :: API AM.AdjacencyMap Ord

adjacencyMapAPI = API

Tests for non-empty modules would need to import API, Generic and perhaps also Generic.NonEmpty (if it turns out to be convenient).

Thanks for the plan! Refactoring the nonempty modules to use generic is a bit daunting, but I'll try to find time and energy soon

@jitwit If you don't want to deal with this daunting task feel free to simply leave a TODO describing the plan. I do understand that it's no fun at all and happy to do it myself :)

snowleopard · 2020-01-22T23:40:55Z

@jitwit Thanks for doing this! I'm a bit terrified about the combinatorial explosion in modules but no idea what to do about it :)

jitwit · 2020-01-23T00:17:25Z

Yeah, the explosion is a bit horrifying. Is that part of why no AdjacencyHashMap type modules are supported?

snowleopard · 2020-01-23T00:23:40Z

@jitwit Yep. At some point, I also had a representation based on arrays but then deleted it too.

jitwit · 2020-05-30T20:01:47Z

Hi and sorry for the MASSIVE delay, I finally returned to this! Hope all is well.
I resolved some merge conflicts and added the last comments you made about the plan for generic tests to test/Algebra/Graph/Test/API.hs

snowleopard · 2020-06-01T22:08:58Z

@jitwit Hey, welcome back! All is good and I hope you're good too. Nice new avatar 👍

The plan looks fine to me, looking forward to future commits :)

jitwit · 2020-06-03T08:37:00Z

Things are still good here, too. Thanks!

zyklotomic · 2021-10-18T15:47:04Z

Hi, I was wondering what the current status of this PR is.

I realized that AdjacencyIntMaps don't have an scc algorithm, which led me to realizing NonEmpty AdjacencyIntMaps were missing too.

Should I try to submit a PR for IntMap scc? It would basically be similar in nature to this PR, copying the code from the regular AdjacencyMap.

Thanks!

snowleopard · 2021-10-18T21:40:15Z

@zyklotomic Hi there! I think @jitwit run out of steam a bit, so it would be great if you could help.

Let's give @jitwit a week to comment. If we don't hear back from him, I think it would be fine to create a new PR, keeping all the commits from this PR and their authorship, and then fixing the remaining issues. I'm happy to review.

zyklotomic · 2021-12-23T09:12:25Z

@snowleopard

I was trying to continue the work mentioned in the thread above #250 (comment) .

If I understand correctly about extending the test API, we would want to add the field

overlays1 :: forall a. c a => NonEmpty (g a) -> g a

which should be analogous to the existing

overlays :: forall a. c a => [g a] -> g a

Furthermore, I interpreted your plan as that we should be adding API's for other types of graphs. I started with nonEmptyAdjacencyMapAPI :: API NAM.AdjacencyMap Ord where NAM is the NonEmpty Adjacency Map module, and
found an issue with implementing the API field for overlays. I can't think of a way that makes sense, since the type [g a] doesn't guarantee that the list will be non-empty.

I think we do in fact need ToGraph instances, because some API entries would be more conveniently implemented
using ToGraph, for example isTopSortOf = ToGraph.isTopSortOf.

I am not sure if I understood your plan correctly, let me know if I have misunderstood it. Thanks!

snowleopard · 2021-12-27T00:40:20Z

@zyklotomic I think you understood the plan correctly. However, implementing a non-generic version of the testsuite for the new module should be OK too: as long as every property we promise in comments is tested, I will be happy :) So, feel free to test some (or all?) of the properties directly, without trying to be too generic. (I admit I might have overengineered the testsuite quite a bit...)

However, since this is all still going to be quite a lot of work, I'd like us to take a step back and think: is adding AdjacencyIntMap.NonEmpty really worth it in the end? This library is already hard to navigate because of so many similar APIs: empty/non-empty, Int/generic, algebraic/Map-based etc.

If I recall correctly, your specific motivation for doing this was the scc function, so here are two questions:

Could you perhaps use AdjacencyMap instead of AdjacencyIntMap? Is the performance boost you get from IntMaps really that important for your application?
If the answer to the above is "yes, I really need the performance of IntMaps": perhaps, it would be better to invest our efforts into making AdjacencyMaps map faster, for example, by implementing them with IntMaps with some auxiliary Int/a mapping.

I am a bit unhappy with how the library looks at the moment (it's way too complex) and I think adding more APIs may be a step in the wrong direction. So, it'd be really interesting to learn more about your motivation/use-case.

zyklotomic · 2022-01-07T22:23:21Z

I have already been using an auxiliary mapping due to space complexity reasons. Performance boost isn't super important.

If you are interested, I used scc here. I was trying to write a heap analysis of the GHC runtime. I used scc to compress cycles in the heap reference graph into one representative node.

The only reason why I thought to add it was because it was missing. I do agree that the library is a bit complex. In my experience, it is because of the inconsistencies (such as scc not being available for IntMaps). Having to select a specific graph "backend" such as AdjacencyMap was confusing. It is still a really enjoyable library to use nonetheless.

snowleopard · 2022-01-11T00:17:34Z

If you are interested, I used scc here. I was trying to write a heap analysis of the GHC runtime. I used scc to compress cycles in the heap reference graph into one representative node.

Wow, that's very cool! Best wishes with finishing this project.

I do agree that the library is a bit complex. In my experience, it is because of the inconsistencies (such as scc not being available for IntMaps). Having to select a specific graph "backend" such as AdjacencyMap was confusing.

Yes, I agree with you. I'd love to have consistency, but it seems almost unreachable in practice because of so many different combinations of graph flavours. It's very hard to implement, document, test and maintain all of these combinations. I'll try to find a way around that, hopefully soon.

jitwit added 5 commits January 22, 2020 15:36

Algebra.Graph.NonEmpty.AdjacencyIntMap compiles

94e88f9

Acked-by: jitwit <[email protected]>

start to sync docs

ef9331c

add test module for NAIM

665c996

fix some names in test module for NAIM

44d5097

gmap

a192362

snowleopard reviewed Jan 22, 2020

View reviewed changes

jitwit commented Jan 22, 2020

View reviewed changes

snowleopard reviewed Jan 22, 2020

View reviewed changes

test/Algebra/Graph/Test/NonEmpty/AdjacencyIntMap.hs Show resolved Hide resolved

snowleopard reviewed Jan 22, 2020

View reviewed changes

jitwit added 3 commits January 22, 2020 19:37

address some comments

28f0018

Merge remote-tracking branch 'upstream/master' into aim-nonempty-go

88cb98a

write out todo plan for moving to generic test API

95dd3f0

zyklotomic mentioned this pull request Oct 31, 2021

NonEmpty AdjancencyIntMaps continued #280

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NonEmpty AdjacencyIntMaps #250

NonEmpty AdjacencyIntMaps #250

jitwit commented Jan 22, 2020

snowleopard Jan 22, 2020 •

edited

Loading

jitwit Jan 23, 2020

snowleopard Jan 22, 2020

jitwit Jan 22, 2020

snowleopard Jan 22, 2020

snowleopard Jan 22, 2020

snowleopard Jan 22, 2020

jitwit Jan 22, 2020

jitwit Jan 22, 2020

snowleopard Jan 22, 2020

snowleopard Jan 22, 2020 •

edited

Loading

jitwit Jan 23, 2020

snowleopard Jan 24, 2020

jitwit Jan 26, 2020

snowleopard Jan 27, 2020

jitwit Jan 30, 2020

snowleopard Jan 30, 2020

snowleopard commented Jan 22, 2020

jitwit commented Jan 23, 2020

snowleopard commented Jan 23, 2020

jitwit commented May 30, 2020

snowleopard commented Jun 1, 2020

jitwit commented Jun 3, 2020

zyklotomic commented Oct 18, 2021

snowleopard commented Oct 18, 2021 •

edited

Loading

zyklotomic commented Dec 23, 2021

snowleopard commented Dec 27, 2021 •

edited

Loading

zyklotomic commented Jan 7, 2022

snowleopard commented Jan 11, 2022 •

edited

Loading

	-- \| The API of 'AM.AdjacencyMap'.
	adjacencyMapAPI :: API AM.AdjacencyMap Ord
	adjacencyMapAPI = API

NonEmpty AdjacencyIntMaps #250

Are you sure you want to change the base?

NonEmpty AdjacencyIntMaps #250

Conversation

jitwit commented Jan 22, 2020

snowleopard Jan 22, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard Jan 22, 2020 • 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

snowleopard commented Jan 22, 2020

jitwit commented Jan 23, 2020

snowleopard commented Jan 23, 2020

jitwit commented May 30, 2020

snowleopard commented Jun 1, 2020

jitwit commented Jun 3, 2020

zyklotomic commented Oct 18, 2021

snowleopard commented Oct 18, 2021 • edited Loading

zyklotomic commented Dec 23, 2021

snowleopard commented Dec 27, 2021 • edited Loading

zyklotomic commented Jan 7, 2022

snowleopard commented Jan 11, 2022 • edited Loading

snowleopard Jan 22, 2020 •

edited

Loading

snowleopard Jan 22, 2020 •

edited

Loading

snowleopard commented Oct 18, 2021 •

edited

Loading

snowleopard commented Dec 27, 2021 •

edited

Loading

snowleopard commented Jan 11, 2022 •

edited

Loading