Repository navigation
Conversation
- Collection only follows direct links (new Node._direct_links) - Node.links is built on _direct_links: same output, each child's links computed once - Add tests for _direct_links, the call count and the unchanged behaviour
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On deeply nested graphs,
Collection.add()andCollection.save()take time that grows exponentially with depth. There are two causes:Collection._add_node()loops overnode.links, which already lists all descendants, and then recurses into each of them, which does the same again.Node.linkscomputes each child's links twice:hasattr(item, "links")evaluates the property, thenitem.linksevaluates it again.Solution
Node._direct_links(new private property): returns only the linked nodes directly referenced by a node. Embedded nodes are looked through.Collection._add_node()andsave()loop over_direct_linksinstead oflinks. The recursion already reaches the deeper levels. This alone fixes both causes 1 and 2.Node.linksis rewritten on top of it. Each child's links are now computed once. This fixes cause 2 for code that still useslinks.Tests on artificial graphs and real datasets
This PR is intended as a refactor only.
linksshould return the same list as before, and aCollectionshould end up with the same nodes, the same blank-node ids and the same order. I checked this by comparing the old and new code on synthetic graphs and on a few real datasets fetched from the EBRAINS KG (v4). For each of them, I loaded it withCollection.load(), added its nodes to a newCollectionand saved it, with the old and the new code (openMINDS 0.6.1).Comparison of the output: the jsonld collections built with the old code and the new code are identical (
diff -r) to the fetched files.Comparison of execution time (load + add + save):
These KG records are rather small and shallow, so the gain is modest. Deeper chains occur naturally in collections built from NWB files with nwb2openminds: one
SubjectStateper session, chained withdescended_from. For one such collection (53 states, longest chain 13), add + save went from 156 s to 0.3 s, with identical output.Tests in
tests/_direct_links, and count_add_nodecalls on a 12-node chain (12 now, 2048 before).test_links_order_and_duplicatesandtest_blank_node_ids_follow_depth_first_order) only show that the behaviour is unchanged, and they pass on the old code too. They check the order oflinks, its duplicates and the numbering of blank-node ids. If these details are not meant to be guaranteed, the two tests can be dropped.