Added sample more feature #133
Conversation
data_broker.js
Outdated
| } | ||
|
|
||
| async _updateStats() { | ||
| async updateStats(useSampleMore = false) { |
There was a problem hiding this comment.
I think this is a pretty confusing interface. A dev would have to look at the code or documentation to understand what calling this method would do.
Rather than making this method public, lets follow the same pattern as the BED file and have a property that triggers an update when set:
Then each broker will have its owner sampling multiple that it keeps track of:
get samplingMultiplier() {
return this._samplingMultiplier;
}
set samplingMultiplier(_) {
this._samplingMultiplier = _;
this._updateStats();
}There was a problem hiding this comment.
Even though it works, I wasn’t convinced this morning either. I had a feeling you wouldn’t be happy with this solution. Your suggestion makes sense. I'll adjust it.
There was a problem hiding this comment.
Sometimes it's better to take a crack at something and see how it goes, so I don't think it was wasted effort.
data_broker.js
Outdated
| @@ -254,7 +254,12 @@ class DataBroker extends EventTarget { | |||
| allRegions = filterRegions(this._bedTextData.regions, validRegions); | |||
| } | |||
|
|
|||
| const regions = sample(allRegions); | |||
| console.log("allRegions", allRegions); | |||
data_broker.js
Outdated
| // Call either sample or sampleMore based on the parameter | ||
| const regions = useSampleMore ? sampleMore(allRegions) : sample(allRegions); | ||
|
|
||
| console.log("sampledRegions", regions); |
sampling.js
Outdated
| function sampleMore (inRegions) { | ||
| if (sampleMultiplier < sampleMultiplierLimit) { | ||
| sampleMultiplier += 1; | ||
| TARGET_BIN_SIZE = TARGET_BIN_SIZE + (TARGET_BIN_SIZE / 4) * sampleMultiplier; |
There was a problem hiding this comment.
TARGET_BIN_SIZE is uppercase because it's a constant. That's a naming convention. If we're going to modify it, it should be targetBinSize. However, we don't want to modify global variables like this. There could be another piece of code in an application that expects the sampling to use the default values, but they will be overwritten if this code is called. Instead of sharing a global, let's keep using the old function and update it to take a multiplier parameter:
function sample(inRegions, multiplier) {
...
}There was a problem hiding this comment.
Before the code review, I want a clarification from you, if we pass the samplingMultiplier into sample() to update the TARGET_BIN_SIZE and NUM_SAMPLES. How about the expandRegion(region), because these two variables are not only used in sample().
There was a problem hiding this comment.
Sorry running behind again. Yeah I think we want to updated the other functions so that we pass down the sampling multiplier
There was a problem hiding this comment.
@YangQi007 this doesn't seem to be triggering more sampling for me
No description provided.