Skip to content
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

Analysis update: Add NBL workflow to README #462

Merged
merged 4 commits into from
Nov 29, 2023
Merged

Analysis update: Add NBL workflow to README #462

merged 4 commits into from
Nov 29, 2023

Conversation

zzgeng
Copy link

@zzgeng zzgeng commented Nov 10, 2023

Purpose/implementation Section

What scientific question is your analysis addressing?

Add NBL workflow to README

What was your approach?

Add NBL workflow to README

What GitHub issue does your pull request address?

Directions for reviewers. Tell potential reviewers what kind of feedback you are soliciting.

Which areas should receive a particularly close look?

NBL workflow

Is there anything that you want to discuss further?

Is the analysis in a mature enough form that the resulting figure(s) and/or table(s) are ready for review?

Yes

Results

What types of results are included (e.g., table, figure)?

What is your summary of the results?

Reproducibility Checklist

  • The dependencies required to run the code in this pull request have been added to the project Dockerfile.
  • This analysis has been added to continuous integration.

Documentation Checklist

  • This analysis module has a README and it is up to date.
  • This analysis is recorded in the table in analyses/README.md and the entry is up to date.
  • The analytical code is documented and contains comments.

@zzgeng zzgeng requested a review from a team as a code owner November 10, 2023 18:56
Copy link

@rjcorb rjcorb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few minor edits and comments:

  • I just want to confirm that the uppermost red box has intended spelling of "neuroblastom" here, and is not a typo
  • in middle red box at pathology free text diagnosis level, I think this should say 'Pathology free text diagnosis is "MYCN non-amp" or...'
  • I'm not sure the "MYCN TPM expression" box is needed here since it isn't a step in the subtyping decision tree, but I understand that it makes the arrows a bit less complicated.
  • Is the suggested cutoff a fixed value, or does it change? If fixed, maybe it should be included here.
  • The NBL subtyping script describes cases where some level of manual review is needed:
If MYCN is called not amplified (i,e., NA, gain, loss, neutral), but the 
clinical data suggests MYCN amplification, plot the CNV data to visualize 
whether we see focal amplification despite CNV not being called

I don't know if this needs to be mentioned in the diagram, or how this is even utilized in making subtype decisions.

  • For cases where there is no TPM value, the subtype is actually reported as "Pathology-amp,MYCN_CN_status-non-amp,TPM-NA", rather than NBL, to be classified. So there may be an additional final subtype box that needs to be included at the bottom.

Copy link

@naqvia naqvia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments. For the filtering step:

  • filter(grepl("neuroblastoma|Neuroblastoma", pathology_diagnosis)), you can add the ignore.case = FALSE to the grepl call.
  • Case 2 needs to be modified in the readme.
  • Red box on flow chart has a typo ("neuroblasom")
  • The Suggested_cutoff needs to be better clarified. Why and how was that chosen? I saw the bar plot and it seems as there was some rationale there but I think it needs to be documented and explained better in the readme, as it is really key component in subtyping.

@zzgeng
Copy link
Author

zzgeng commented Nov 20, 2023

Looking good, just a few minor edits and comments:

  • I just want to confirm that the uppermost red box has intended spelling of "neuroblastom" here, and is not a typo
  • in middle red box at pathology free text diagnosis level, I think this should say 'Pathology free text diagnosis is "MYCN non-amp" or...'
  • I'm not sure the "MYCN TPM expression" box is needed here since it isn't a step in the subtyping decision tree, but I understand that it makes the arrows a bit less complicated.
  • Is the suggested cutoff a fixed value, or does it change? If fixed, maybe it should be included here.
  • The NBL subtyping script describes cases where some level of manual review is needed:
If MYCN is called not amplified (i,e., NA, gain, loss, neutral), but the 
clinical data suggests MYCN amplification, plot the CNV data to visualize 
whether we see focal amplification despite CNV not being called

I don't know if this needs to be mentioned in the diagram, or how this is even utilized in making subtype decisions.

  • For cases where there is no TPM value, the subtype is actually reported as "Pathology-amp,MYCN_CN_status-non-amp,TPM-NA", rather than NBL, to be classified. So there may be an additional final subtype box that needs to be included at the bottom.

Thank you for your suggestion!
I have fixed typos. For cutoff, I added a note under the figure, which hopefully could help people understand how it works. I also see the plots were generated for CNV visualization, but I could not find the code to use the results of the plot for subtyping. Last, I think in latter script, they changed "Pathology-amp,MYCN_CN_status-non-amp,TPM-NA"toNBL, to be classified(https://github.com/d3b-center/OpenPedCan-analysis/blob/0c73b82204123dfe917f6019a0a3095e861e2b8e/analyses/molecular-subtyping-NBL/04-subtyping.Rmd#L259C1-L263C47). So in final table, we only haveNBL, to be classified`.

@jharenza jharenza requested a review from rjcorb November 25, 2023 21:38
Copy link
Member

@jharenza jharenza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might suggest instead of "No TPM value" saying "No RNA-Seq" here, since that is the only reason we would not have a TPM value, and maybe just making it a black box with white background?

There is a star here denoting a cutoff but no cutoff listed- can you put a star with a cutoff somewhere? Maybe near the legend, under the RNA expression?

@zzgeng
Copy link
Author

zzgeng commented Nov 27, 2023

I might suggest instead of "No TPM value" saying "No RNA-Seq" here, since that is the only reason we would not have a TPM value, and maybe just making it a black box with white background?

There is a star here denoting a cutoff but no cutoff listed- can you put a star with a cutoff somewhere? Maybe near the legend, under the RNA expression?

Thank you for the suggestion! I have changed the figure and add note in figure and more detailed note in README after this figure.

Copy link
Member

@jharenza jharenza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good now!

@jharenza
Copy link
Member

docs only, no need for GA

@jharenza jharenza merged commit 341f8f6 into dev Nov 29, 2023
0 of 22 checks passed
@jharenza jharenza deleted the add_NBL_workflow branch November 29, 2023 22:02
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants