search

dondi / GRNsight

Web app and service for modeling and visualizing gene regulatory networks.
http://dondi.github.io/GRNsight
BSD 3-Clause "New" or "Revised" License
17 stars 8 forks source link

Minimum weight should always be zero #515

Closed kdahlquist closed 7 years ago

kdahlquist commented 7 years ago

This was uncovered in issue #513. The minimum for the range of edge weights should always be zero, not the smallest weight in the worksheet.

dondi commented 7 years ago

The current beta code reads like this: https://github.com/dondi/GRNsight/blob/beta/web-client/public/js/graph.js#L67-L72

  //normalization all weights b/w 2-14
  var normMax = +$("#normalization-max").val();
  var totalScale = d3.scale.linear()
    .domain(normalization && normMax > 0 ? [0, normMax] : d3.extent(allWeights))
    .range([2, 14])
    .clamp(true);

So the minimum on beta is zero only if a custom normalization value has been set. It appears that zero-as-minimum was misunderstood—instead of always being the minimum, it is only the minimum when there is no normalization value.

In any case setting zero as the minimum all the time should be straightforward. Instead of using extent, we should just use max:

  //normalization all weights b/w 2-14
  var normMax = +$("#normalization-max").val();
  var edgeMax = normalization && normMax ? normMax : d3.max(allWeights);
  var totalScale = d3.scale.linear()
    .domain([0, edgeMax])
    .range([2, 14])
    .clamp(true);
mihirsamdarshi commented 7 years ago

Huh, seems to be not as simple as that, it seems that an error in d3 comes up

kdahlquist commented 7 years ago

I think this may be the source of the issue reported in #437.

dondi commented 7 years ago

A fix for this has been pushed to beta; it’s ready for review.

I’ve found that the files in test-files/graph-tests appear to be the best ones for noticing what’s different. The all--weights.xlsx files are pretty good, as is different-edge-widths.xlsx.

kdahlquist commented 7 years ago

I have confirmed that this is now fixed on beta. However, see my comment on #406 about being vigilant about commenting on that issue when beta is refreshed and updating the version number and date.

The test files that @dondi mentioned are OK for testing this functionality. However, I am appending a file that specifically tests just this issue below. Check it out to see that it gives different results on the current master and beta servers. Would someone please commit this test file to the repo? When that is done, this issue can be closed.

compare-edge-thickness-0.5-1.xlsx

kdahlquist commented 7 years ago

@dondi committed the file.