Closed aleclearmind closed 2 years ago
@aleclearmind — this is a great addition, thank you!! I am in a time-crunch for the next few hours, but will review more closely later today (GMT-5)!
Merging #27 (5a4d88c) into master (79da380) will increase coverage by
0.77%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #27 +/- ##
==========================================
+ Coverage 98.05% 98.82% +0.77%
==========================================
Files 5 4 -1
Lines 513 509 -4
==========================================
Hits 503 503
+ Misses 10 6 -4
Impacted Files | Coverage Δ | |
---|---|---|
grandiso/__init__.py | 95.31% <100.00%> (ø) |
|
setup.py |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 79da380...5a4d88c. Read the comment docs.
@aleclearmind this looks great to me — thank you so much for this contribution!! are you ready to merge?
Sure, go ahead! Can I ask you to make a release? This would save us from using a fork in our own project: we use it to compare if a YAML document with references is a subset of another YAML document and being able to point the algorithm towards the nodes with the highest disambiguation power makes the whole thing go from 4m down to 0.5s.
Awesome! Packaging it up now :)
@aleclearmind — just letting you know this is now installable from pypi!
Thanks a lot!
The
interestingness
parameter was being used only to initialize the firstnext_node
and nowhere else. Also, the way it was used was flawed since it was picking the first key of the dictionary, which is not guaranteed to be the one with the highest interest.This commit properly uses
interestingness
both for choosing the first node and to pick the best candidate fornext_node
, when more than on is available.