Eliding small snarls #374
Eliding small snarls #374
Conversation
…'s what the user requests
…ng_small_snarls merging master into this branch
|
@shreyasun It looks like you need to add |
public/help/help.md
Outdated
| @@ -27,5 +27,6 @@ The following procedure describes adding and updating settings of custom tracks. | |||
| * A start position and a distance (e.g. "chr1:1+100") | |||
| * A node ID anchor and a distance (e.g. "node:100+10") | |||
|  | |||
| 9. Click Go to see the selected tracks render in the visualization area. | |||
| 4. There is an option to click on the "Simplify Off" button to enable vg simplify, which would remove eliding small snarls in the graph. This would only work for BED files or graph tracks (not reads or haplotypes). | |||
There was a problem hiding this comment.
This makes it sound like you can turn on simplification for all BED file chunks. It also reads like the user should expect to see a non-functional simplify button sometimes, when actually the button only appears when it is going to work.
I think we want to communicate that the button is not always visible (so people aren't surprised when they don't see it), and that people should expect to see it only when there aren't any reads to be displayed.
I think it does work with haplotype tracks; the haplotype information goes into vg chunk and the vg simplify pass happens after that.
src/common.mjs
Outdated
| @@ -111,6 +111,16 @@ export function defaultTrackColors(trackType){ | |||
| } | |||
| } | |||
|
|
|||
| /* Function to determine if reads are added, where tracks is an object */ | |||
There was a problem hiding this comment.
Tracks isn't just any object, right? It is meant to be an object full of track objects (which is a type we've defined).
And by "reads are added", we mean that we're checking to see if any of the tracks is a read track. I'm not sure "added" is the right word here; we're checking to see if they are present.
src/components/CopyLink.js
Outdated
| let resultSimplify = result.simplify; | ||
| if (resultSimplify === "true"){ | ||
| result.simplify = true; | ||
| } else if (resultSimplify === "false"){ | ||
| result.simplify = false; | ||
| } |
There was a problem hiding this comment.
I think the resultSimplify variable could be eliminated here to improve the code.
src/components/HeaderForm.js
Outdated
| @@ -713,6 +718,11 @@ class HeaderForm extends Component { | |||
| }; | |||
| }; | |||
|
|
|||
| /* Function for simplify */ | |||
| enableSimplify = () => { | |||
There was a problem hiding this comment.
This doesn't actually "enable" simplification; it toggles it. I think toggling it is the correct behavior so the function name should be changed.
src/components/HeaderForm.js
Outdated
| @@ -713,6 +718,11 @@ class HeaderForm extends Component { | |||
| }; | |||
| }; | |||
|
|
|||
| /* Function for simplify */ | |||
There was a problem hiding this comment.
This comment doesn't do very much to explain the function. It should say what the function needs to do. Or you could say this is the setter function for the simplify two-way binding to the button and let that imply what it is supposed to do.
src/components/HeaderForm.js
Outdated
| @@ -752,6 +762,9 @@ class HeaderForm extends Component { | |||
| const examplesFlag = this.state.dataType === dataTypes.EXAMPLES; | |||
| const viewTargetHasChange = !viewTargetsEqual(this.getNextViewTarget(), this.props.getCurrentViewTarget()); | |||
|
|
|||
| const ifReadsExist = readsExist(this.state.tracks); | |||
There was a problem hiding this comment.
It looks like this variable is sued exactly once, so maybe the function call should just be made where the variable is used?
Or if you need the variable to simplify the complicated logic where it is used, maybe we could move the variable closer to the logic it goes with?
src/server.mjs
Outdated
| VG_PATH + | ||
| "vg " + | ||
| "simplify " + | ||
| "- " + |
There was a problem hiding this comment.
This doesn't accurately report the command that was called; when we run vg simplify here we give it a filename as an argument and not -.
src/server.mjs
Outdated
| console.log(`vg simplify exited with code ${code}`); | ||
| vgViewCall.stdin.end(); | ||
| if (code !== 0) { | ||
| console.log("Error from " + VG_PATH + "vg " + "simplify - "); |
There was a problem hiding this comment.
Here also we're incorrectly claiming that we called vg simplify -.
Fixes #196