fix: #265 and #261 (/js/ldavis.v3.0.0.js)
#266
Conversation
ed9w2in6
changed the title
/js/ldavis.v3.0.0.js)/js/ldavis.v3.0.0.js)
|
@msusol @bmabey Hi, I would love to see this being merged since apparently in modern browsers Javascript files cannot be served without a server. This means I cannot use this fix myself. Let me know if you would need any help. Of course, in the long run we should decouple the numbering from labels such that feature request like #92 can be implemented. Decoupling should also remove the need of |
pyLDAvis/js/ldavis.v3.0.0.js
Outdated
| @@ -178,7 +182,7 @@ var LDAvis = function(to_select, data_or_file_name, color1, color2) { | |||
| if (termElem !== undefined) term_off(termElem); | |||
| vis_state.term = ""; | |||
| var value_old = document.getElementById(topicID).value; | |||
| var value_new = Math.min(K, +value_old + 1).toFixed(0); | |||
| var value_new = Math.min(K - 1 + startIndex, +value_old + 1).toFixed(0); | |||
There was a problem hiding this comment.
K - 1 + startIndex -> K + startIndex - 1
Just to be consistent everywhere
There was a problem hiding this comment.
added commit 0a96505
| value_new = Math.min(K, Math.max(1, value_new)); | ||
| // older versions check for value_new !== "" too, why? | ||
| if (!isNaN(value_new) && value_new > startIndex - 1) { | ||
| value_new = Math.min(K + startIndex - 1, Math.max(startIndex, value_new)); |
There was a problem hiding this comment.
added commit 0a96505
| @@ -512,7 +517,7 @@ var LDAvis = function(to_select, data_or_file_name, color1, color2) { | |||
| .attr("x", 0) | |||
| .attr("y", mdsheight + 10 + (8/2)*barguide.height + 5) | |||
| .style("dominant-baseline", "middle") | |||
| .text("2. relevance(term w | topic t) = \u03BB * p(w | t) + (1 - \u03BB) * p(w | t)/p(w); see Sievert & Shirley (2014)"); | |||
| .text("2. relevance(term w | topic t; \u03BB) = \u03BB * log(p(w | t)) + (1 - \u03BB) * log(p(w | t)/p(w)); see Sievert & Shirley (2014)"); | |||
There was a problem hiding this comment.
You don't mention this formula change?
There was a problem hiding this comment.
I changed it at line 511. I am not sure what you mean.
| @@ -512,7 +517,7 @@ var LDAvis = function(to_select, data_or_file_name, color1, color2) { | |||
| .attr("x", 0) | |||
| .attr("y", mdsheight + 10 + (8/2)*barguide.height + 5) | |||
| .style("dominant-baseline", "middle") | |||
| .text("2. relevance(term w | topic t) = \u03BB * p(w | t) + (1 - \u03BB) * p(w | t)/p(w); see Sievert & Shirley (2014)"); | |||
| .text("2. relevance(term w | topic t; \u03BB) = \u03BB * log(p(w | t)) + (1 - \u03BB) * log(p(w | t)/p(w)); see Sievert & Shirley (2014)"); | |||
There was a problem hiding this comment.
Why is this change here?
relevance(term w | topic t; \u03BB)
There was a problem hiding this comment.
The notation is to indicate that \lambda is a parameter of the metric, such that the formula aligns better to the one shown in the linked paper:
fixes #265 and #261
There does not seems to be any testing mechanism for the javascript code so no test are added.
Tested locally myself, matches my own expectation as described in #265.
@bmabey Please check.