-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SARS-CoV-2 verison of RAMPART #1
base: master
Are you sure you want to change the base?
Conversation
server/config/pipeline.js
Outdated
verbose("config", `Limiting barcodes to: ${pipeline.configOptions.limit_barcodes_to}`) | ||
// if any samples have been set (and therefore associated with barcodes) then we limit the run to those barcodes | ||
if (config.run.samples.length) { | ||
if (pipeline.configOptions.limit_barcodes_to) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did the indentation here change? don't they have some prettier set up to automatically format files? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, my mistake. I'll fix it.
server/config/pipeline.js
Outdated
queue: true | ||
}); | ||
} | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep style of old code. Originally it was } else {
. Same for the else if above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay.
server/config/getInitialConfig.js
Outdated
pathCascade.push(normalizePath(userProtocolPath)); | ||
} | ||
|
||
pathCascade.push("./"); // add current working directory | ||
|
||
//log("path cascade:"+pathCascade); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, remove all stray commented-out code which was introduced
server/PipelineRunner.js
Outdated
job.sample_name = sampleName; | ||
job.barcodes = global.datastore.getBarcodesForSampleName(sampleName); | ||
job.samples = `{${job.sample_name}: [${job.barcodes}]}`; | ||
if(sampleName){ //run-per-sample |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this if needed? what will happen otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does this happen only in our modification of the code or in general? What are the exact circumstances the function is called with empty sampleName?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the sampleName is relevant only when we want to create a job for run-per-sample pipeline. In original rampart version, only the annotation pipeline was not a run-per-sample one. we created another pipeline, which is not run-per-sample, but we want to be able to trigger it manually from UI, not automatically when the server starts. Therefore we added this if - the createJob method is used in listeners defined in socket.js
, where we are adding a job to queue. Run-per-sample pipelines will not be affected by this change. When we would like to add another pipeline which should be run on all samples, we can reuse this modified createJob method. When the pipeline is to be run for all the samples, we dont have any sampleName defined, so that the call of global.datastore.getBarcodesForSampleName(sampleName)
method will fail, and we will get the fatal error mentioned above - and calling this method is irrelevant for pipeline which is not run-per sample... if you dont need the set of barcodes for a specific sample, for some reason - but if you do, you can pass a non-null sampleName to the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so perhaps we can clarify, before the if statement
// only pipelines which are run per sample have sampleName set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we can continue, we should
- remove all newly-introduced stray commented-out code
- keep originaly style&formatting. (I am actually amazed they don't have prettier set up)
- keep default pipeline&default configuration unchanged; remove custom configuration (that is use case specific, not RAMPART specific
default_protocol/genome.json
Outdated
@@ -1,11 +1,46 @@ | |||
{ | |||
"label": "default", | |||
"length": 1000, | |||
"label": "SARS-CoV-2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we override default protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is related to my comment regarding custom configuration. This content is readily available in nanocrop. Users should not fetch rampart with our custom stuff instead of default example that is related to rampart documentation.
@@ -0,0 +1,11 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this RAMPART repository should hold no custom configuration. All the custom protocols and run_configuration files should be located in nanocrop, where we have developed terminal user interface to let the user select from several protocols etc. In RAMPART should only be default examples of protocols and run configurations as they are in original repository. This fork should implement strictly RAMPART enhancements, such as new pipeline and internal modifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, all custom configuration except the new pipeline itself is removed. However, I think that leaving the default ebola examaples in the repository might be misleading for now, because the new pipeline, now still located in default protocol, contains a virus-specific file which describes mutations for the SARS-CoV-2 variants we are matching. I think we should then consider moving the pipeline itself to custom protocols in nanocrop too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fixed mechanism in RAMPART that default pipeline is loaded from default_protocol/ folder or can it be configured somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is possible to change it by changing one constant in RAMPART code (DEFAULT_PROTOCOL_PATH in server/config/getInitialConfig.js). It is not configurable form command line or config file, i guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but you can add a new pipeline to RAMPART by creating a pipelines.json
file in your custom protocol folder. so we can remove our strand-matching pipeline from the default protocol and add it to your config in nanocrop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thak you! That's a way to go, I think. Firstly I agree that Ebola examples might be misleading when default pipeline is heavily focused on SARS-CoV-2. But I think that default pipeline should remain the original default pipeline, not our SARS-CoV-2 focused one. Original default pipeline is much more versatile, intuitive, well documented and can be used for any sequenced genome once a proper reference file is in particular used protocol. So I would place default pipeline to default_protocol as it was. I also don't think that default_protocol folder is well named. Because it contains default_protocol together with default pipeline. Now that we have more pipelines (and possibly more coming) I would create "pipelines/" folder in repository root a there folders "default_pipeline" and folder for our pipeline (you can come up with better name than me :) ). And default protocol would hold just default_protocol. pipelines.json file in "default_protocol/" would have modified path to point to newly placed default pipeline.
As you suggested, the right pipeline to be used for our experiment would be specified in nanocrop.
When it comes to example stuff, I would like to preserve them (default pipeline will support them). It just boosted my learning curve with RAMPART so much when I used it for the first time. And we have to support newcomers in my opinion. It doesn't matter that it's Ebola genome, it's just a few clicks and you can run RAMPART and see what it does, that's very useful content from original repository in my opinion. So I would preserve example fastq files and also I wouldn't move examples to "old/" folder because they seem outdated there. I would leave them without change. But I can be wrong. Feel free to share your thoughts @ppershing @janka000 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having example fastq files was also very helping when I found out that fastq files generated by our compbio basecallers were not compatible with RAMPART. It was very easy to compare them against example file and identify missing metadata in our generated fastq.
I found file default_protocol/pipelines/run_python_scripts/rules/pycache/helpers.cpython-38.pyc in changed files. I'm not sure but it seems to me like some IDE generated file. Should it be here? |
the file default_protocol/pipelines/run_python_scripts/rules/__pycache__/helpers.cpython-38.pyc is generated by python interpreter, it should not be there.. I'll remove it and add the __pycache__ folder to .gitignore. |
##### Target rules ##### | ||
|
||
rule all: | ||
input: | ||
expand(config["output_path"]+ "/{filename_stem}.csv", filename_stem=config["filename_stem"]) | ||
|
||
csv = expand(config["output_path"]+ "/{filename_stem}.csv", filename_stem=config["filename_stem"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes to default pipeline seem to me like we just forgot about them. We don't use the csv variable or do we? Same would apply to default_pipeline/demux_map/config.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, we don't. fixed :)
} | ||
let variant_name=""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shorter version:
let variant_name = sampleVariant.map(variant => variant.name).join(" ")
src/components/SamplePanel/index.js
Outdated
const chartsToShow = showSinglePanel ? | ||
charts[showSinglePanel] : | ||
[charts.coverage, charts.readLength, charts.coverageOverTime, charts.refSimilarity]; | ||
var chartsToShow =[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is misleading that we first assign an array to it, then, later in ternary operator it can be wihout array.
Also, do not use var
, prefer const
or let
if it needs to be mutable
src/components/SamplePanel/index.js
Outdated
chartsToShow = showSinglePanel ? charts[showSinglePanel] : [charts.coverage, charts.readLength, charts.coverageOverTime, charts.refSimilarity, charts.mutationsTree]; | ||
} | ||
else{ | ||
chartsToShow = showSinglePanel ? charts[showSinglePanel] : [charts.coverage, charts.readLength, charts.coverageOverTime, charts.refSimilarity]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels a bit copy-paste. I guess it would better read
if (showSinglePanel) {
chartsToShow = charts[...]
} else {
chartsToShow = [coverage, ..., coverageOverTime]
if (variant_name !== "") {
chartsToShow.push(charts.mutationsTree)
}
}
} | ||
`; | ||
|
||
export default SmallerModernButton; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
many files here are missing final newline. It is usually a good custom to configure your editor to save final newline. Can you check rest of the project if they have newlines and if yes, fix newly introduced inconsistencies?
} | ||
let variant_name = sampleVariant.map(variant => variant.name).join(" "); | ||
if(variant_name===""){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could be tested before and assignedi n a single expression, e.g.
const variant_name = sampleVariant.length > 0 ? sampleVariant.map().join() : "not known yet"
src/components/SamplePanel/index.js
Outdated
@@ -74,6 +75,13 @@ const SamplePanel = ({sampleName, sampleData, config, reference, socket, panelEx | |||
const sampleColours = {}; /* dataformat needed by <CoveragePlot> */ | |||
sampleColours[sampleName] = sampleColour; | |||
|
|||
var variant_name=""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we again can prefer .map.join()
. Also, var
is not good, prefer const/let
} | ||
|
||
componentDidUpdate(prevProps) { | ||
this.render(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not update variants tree if props change. Are we sure we want that? Also, can't this be written in a functional-component with perhaps useMemo()
hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to do this :/. I am quite a newbie to react.
but the variants tree needs to be updated only when the Match strands pipeline is triggered and produces a new json file - as far as I tested it, it worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppershing Could we ask you to help us with this, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something along lines
function computeVariantsTree(data) {
...
}
type Props = {...}
const MutationTree = (props: Props) => {
const variantsTree = React.useMemo(computeData(props.data), [props.data])
return <Container> {variantsTree} </Container>
}
Also
- data probably needs better name
- renderProp is bad name as well. Maybe "additionalContent" would be better?
|
||
createVariants(variants, level){ | ||
let variantLines=[]; | ||
for(const variant of variants){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be rewritten in a more functional way like this
return variants.flatMap((variant) => [
`${'---'.repeat(level)}variant: ${variant.name} (${mutations})`,
...createVariants(variant.subs, level+1)
])
server/startUp.js
Outdated
@@ -23,7 +23,8 @@ async function getCSVs(dir) { | |||
const dirents = await readdir(dir, { withFileTypes: true }); | |||
const files = await Promise.all(dirents.map((dirent) => { | |||
const res = path.resolve(dir, dirent.name); | |||
return dirent.isDirectory() ? getCSVs(res) : res; | |||
//return dirent.isDirectory() ? getCSVs(res) : res; | |||
return dirent.isDirectory() ? "ignore" : res; //dont look for csv files recursively - look only for the files that RAMPART created in annotations pipeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ignore" is not particularly nice. Maybe return null instead?
Also, do we want to keep commented out getCSVs line?
server/datastore.js
Outdated
@@ -254,6 +257,23 @@ const whichReferencesToDisplay = (dataPerSample, threshold=5, maxNum=10) => { | |||
return refMatchesAcrossSamples; | |||
}; | |||
|
|||
function getSampleVariants(){ | |||
let fileToParse = global.config.run.annotatedPath+"results/mutations.json"; | |||
let variantData ={}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incinsistent spacing areound = (applies to the whole file)
server/config/pipeline.js
Outdated
@@ -73,6 +72,18 @@ function setUpPipelines(config, args, pathCascade) { | |||
}, | |||
queue: true | |||
}); | |||
} else if(key == "barcode_strand_match"){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer === for string comparisons
server/config/pipeline.js
Outdated
@@ -188,7 +199,7 @@ function checkPipeline(config, key, pipeline, giveWarning = false) { | |||
} | |||
|
|||
/* deprecation warnings */ | |||
if (pipeline.requires && key !== "annotation") { | |||
if (pipeline.requires && key !== "annotation" && key!=="barcode_strand_match") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inconsistent spacing
|
||
|
||
def check_variant(barcode, threshold, strand): #check whether this variant meets our conditions | ||
pocet=0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
english please
|
||
def check_variant(barcode, threshold, strand): #check whether this variant meets our conditions | ||
pocet=0 | ||
json_string = '{' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you build json string instead of building dict and then using json.dumps()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because I did not know of the json.dumps()
method. I should have googled it in the first place.. Thanks. I'll use it.
yield from load_fasta_fd(f) | ||
|
||
|
||
def load_fasta_fd(f): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably call this iter_fasta_fd to indicate that this is lazy evaluated
letters = "ACGT" | ||
l2n = {letter: num for num, letter in enumerate(letters)} | ||
|
||
def load_dict(file_path, reference): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this same as in compare_mutations.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is. I'll import it to compare_mutations from helpers. same in count_observed_counts.
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit too many stray lines at the end
|
||
def create_bam(reference,fname,fastq_path,working_path): | ||
print("minimap2 -t 2 -x map-ont -a "+reference+" "+fastq_path+"/"+fname+".fastq | samtools view -S -b -o - | samtools sort - -o "+working_path+"/"+fname+".bam && samtools index "+working_path+"/"+fname+".bam") | ||
res=os.system("minimap2 -t 2 -x map-ont -a "+reference+" "+fastq_path+"/"+fname+".fastq | samtools view -S -b -o - | samtools sort - -o "+working_path+"/"+fname+".bam && samtools index "+working_path+"/"+fname+".bam" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os.system is dangerous as it invokes shell + this does not handle escaping arguments well. prefer `subprocess.run(['binary', 'list', 'of', 'arguments'])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, something like this?
p1 = subprocess.Popen(["minimap2", "-t 2", "-x", "map-ont", "-a", reference, fastq_path+"/"+fname+".fastq" ], stdout=subprocess.PIPE)
p2 = subprocess.Popen(["samtools", "view", "-S", "-b", "-o", "-" ], stdin=p1.stdout, stdout=subprocess.PIPE)
subprocess.run(["samtools", "sort", "-", "-o "+working_path+"/"+fname+".bam" ], stdin=p2.stdout)
subprocess.run(["samtools", "index", working_path+"/"+fname+".bam"])
I am not sure how to implement it correctly using subprocess
(I've tried this, but it did not create the .bam file, but i got no error)
|
||
return ( | ||
<SmallerModernButton onClick={send}> | ||
<IoMdPlay/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has some weird indentation
src/components/SamplePanel/index.js
Outdated
@@ -74,6 +75,10 @@ const SamplePanel = ({sampleName, sampleData, config, reference, socket, panelEx | |||
const sampleColours = {}; /* dataformat needed by <CoveragePlot> */ | |||
sampleColours[sampleName] = sampleColour; | |||
|
|||
const variant_name=sampleVariant.map(variant => variant.name).join(" "); | |||
|
|||
const panelExpandedHeight = (variant_name==="") ? "370px" : "740px"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's be consistent with spacing around ===
} | ||
|
||
componentDidUpdate(prevProps) { | ||
this.render(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something along lines
function computeVariantsTree(data) {
...
}
type Props = {...}
const MutationTree = (props: Props) => {
const variantsTree = React.useMemo(computeData(props.data), [props.data])
return <Container> {variantsTree} </Container>
}
Also
- data probably needs better name
- renderProp is bad name as well. Maybe "additionalContent" would be better?
server/startUp.js
Outdated
})); | ||
return Array.prototype.concat(...files) | ||
.filter((f) => f.endsWith('.csv')); | ||
.filter((f) => (f!=null && f.endsWith('.csv'))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
server/datastore.js
Outdated
@@ -302,23 +324,30 @@ Datastore.prototype.getDataForClient = function() { | |||
/* Following removed as the client no longer uses it - Mar 30 2020 */ | |||
// refMatchSimilarities: sampleData.refMatchSimilarities | |||
} | |||
|
|||
if(!(sampleName in variantData)){ | |||
variantData[sampleName] =[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
return variantData; | ||
} | ||
let file_content = fs.readFileSync(fileToParse).toString(); | ||
vData = JSON.parse(file_content); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forgotten let/const?
server/datastore.js
Outdated
@@ -254,6 +257,23 @@ const whichReferencesToDisplay = (dataPerSample, threshold=5, maxNum=10) => { | |||
return refMatchesAcrossSamples; | |||
}; | |||
|
|||
function getSampleVariants(){ | |||
let fileToParse = global.config.run.annotatedPath+"results/mutations.json"; | |||
let variantData = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be const, the same on line above as well
server/datastore.js
Outdated
let file_content = fs.readFileSync(fileToParse).toString(); | ||
vData = JSON.parse(file_content); | ||
|
||
for(const barcode of vData){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing for (
and I am not sure about ) {
(check rest of the project)
server/config/pipeline.js
Outdated
@@ -73,6 +72,18 @@ function setUpPipelines(config, args, pathCascade) { | |||
}, | |||
queue: true | |||
}); | |||
} else if(key === "barcode_strand_match"){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spacing
return barcode_dict | ||
|
||
|
||
def dump_dict_to_file(counts, f): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function smells like you want to use csv writer
|
||
def create_barcodes_dict(csv_filename): | ||
barcode_dict = {} | ||
with open(csv_filename, "r") as f: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it is csv, why don't we use csv parser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we want to create a dictionary like {"read_name":"barcode","read_name2":"barcode2"... }
from it, to be able to match barcodes to read names quickly. I think this is not what csv parser does/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not saying that the output is 1:1 of the csv parser. Rather, that we should use csv reader to "split" line to individual pieces instead of doint it manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can actually use DictReader and it would be more natural than assigning arr[some_abritrary_looking_number]
<Title> | ||
{"Mutations tree"} | ||
</Title> | ||
{this.createVariants(this.props.data, 0)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this? isn't it the simplest way how to achieve what we want, without using hooks, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, assuming this is performant enough (I have no idea whether the caching in previous version was because of performance or not).
Anyway, this looks good, let's just rename it to renderVariants
|
||
componentDidMount() { | ||
let variantsTree = this.createVariants(this.props.data, 0).map(i => { | ||
])).map(i => { | ||
return <p>{i}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't react complaining that array elements don't have any key
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it doesn't, as far as I can see
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? Doesn't it output warnings to the console in the runtime? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I see only the log outputs which were there in original rampart too.
|
||
componentDidMount() { | ||
let variantsTree = this.createVariants(this.props.data, 0).map(i => { | ||
])).map(i => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this correct? createVariants is recursive, and this therefore <p>
escapes recursively. Is that what we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right, this might be a problem. maybe if we seperated the second map to the second function, it might by okay.
barcode_dict = {} | ||
with open(csv_filename, "r") as f: | ||
line = f.readline() | ||
while True: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if we don't use csv reader, why not for line in f
construct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, we could use the for line in f
construct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I know, you can mix f.readline()
and for line in f
. But if you really want to skip first line (that is, header), csv DictReader makes much more sense
Version of RAMPART designed for sequencing SARS-CoV-2 virus - we added a new pipeline to determine the variants of the sequenced samples