search

cdd / bioassay-template

Other
7 stars 2 forks source link

Issue #791: added method addNode, to be used in BAE for tacking on provisional terms to schema. #86

Closed kodecharlie closed 6 years ago

kodecharlie commented 6 years ago

Let me know if you think I should scratch up a unit-test for this new method.

aclarkxyz commented 6 years ago

Adding one node at a time might not be the way to go here (and it could even become a performance burden). A better strategy might be to have an entrypoint that takes an array of new node candidates, and then:

  1. iterate over the set
  2. if there's a parent available, update the tree hierarchy
  3. if loop ran through with no changes, exit (anything remaining is invalid)
  4. recalculate flat/list and update all of the parent indices

Keeping in mind that order of addition is important: sometimes the set will contain items that are children of other items, and order must be assumed random.

You could pre-filter the set by eliminating anything whose parent is neither in the tree nor in the list of things to add, which means that any SchemaTree for which none of the additions apply will be skipped over quickly

kodecharlie commented 6 years ago

@gedeck -- it's clear which code your comments refer to; it would even be more clear if your comments were interwoven with highlighted logic in git's code-review page... My responses: testAddNodeWithMissingParent vets the unusual scenario in which a new provisional term is submitted which in fact has no parent; I thought it was important to test this case explicitly. Also, I like your proposal for verifyNodeInCollection and will incorporate it shortly.

@aclarkxyz -- my impression from discussions was that adding provisional terms is NOT a high-frequency transaction. So to my thinking, it makes sense for SchemaTree.addNode to take a single node parameter. If you foresee different behavior, then I'll support a collection parameter, and generally, I would agree with your assertions on performance improvements (over the single-parameter method).

ALSO: it's clear there will be at least one further modification to the Node data structure that would indicate whether it is a provisional term or not. I will get around to that change once I commence work on the UI.

kodecharlie commented 6 years ago

@aclarkxyz -- so, I think there are two distinct use-cases: (i) at "boot-up", when we load a collection of provisional terms archived in the database; (ii) when we populate a new provisional term from the UI.

I see now why a version of addNode with a collection parameter would be much more efficient at start-up. Let me look into this today.