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

ChIPSeq Template Created #543

Merged
merged 8 commits into from
Nov 18, 2024
Merged

ChIPSeq Template Created #543

merged 8 commits into from
Nov 18, 2024

Conversation

changtotheintothemoon
Copy link
Contributor

Hi @allaway @anngvu

I created a new initial branch for adding a new template for the ChIP-Seq assay:
Related Issue: #532

Please review the template and let me know if there are additional sections that need to be added.

@allaway mentioned creating a new section for the antibody in modules/Sample/SpecimenProcessing.yaml. I have not modified this yet.
He suggested using the OLS/NCIT definition for the ChIP-Seq Antibody Name: NCIT_C177613

@anngvu, what are your suggestions regarding adding the antibody to the template?

Thanks,
James

Copy link

github-actions bot commented Nov 12, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2024-11-18 15:55 UTC

modules/Template/Data.yaml Outdated Show resolved Hide resolved
is_a: EpigenomicsAssayTemplate
description: Template for describing raw data from ChIP-Seq assays, typically used to identify DNA-protein interactions.
slots:
- antibodyID
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can add a more specific ChIPseqAntibodyName property with the NCIT meaning as suggested and use it here. We can either keep it free text or import a list of ChIPseq antibody names, but I'd prefer people use the antibodyID as the preferred field anyway -- you can make antibodyID required. For additional context, it looks like HTAN just uses single free text antibodyName primarily https://github.com/ncihtan/data-models/blob/4b9bb3cfcbb979694479a4a60b7dac71becfbf96/HTAN.model.csv#L226

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we make this antibodyID slot required, would this also effect other templates that might been using the slot?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, because the feature to set contextual required is not implemented in schematic yet. In this case, I think it's OK to make it required across all templates that use it -- you would only add antibodyID to the template if you really needed it anyway.

@anngvu anngvu self-requested a review November 12, 2024 22:27
Copy link
Collaborator

@anngvu anngvu left a comment

Choose a reason for hiding this comment

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

Update props.yaml

Copy link

github-actions bot commented Nov 12, 2024

Test Suite Report 24.7.2

Template Generation

template result link
ChIPSeqTemplate 😄 template link
ClinicalAssayTemplate 😄 template link
EpigeneticsAssayTemplate 😄 template link
FlowCytometryTemplate 😄 template link
GenomicsAssayTemplate 😄 template link
GenomicsAssayTemplateExtended 😄 template link
HumanCohortTemplate 😄 template link
ImagingAssayTemplate 😄 template link
LightScatteringAssayTemplate 😄 template link
MethylationArrayTemplate 😄 template link
MRIAssayTemplate 😄 template link
PharmacokineticsAssayTemplate 😄 template link
PlateBasedReporterAssayTemplate 😄 template link
ProcessedAlignedReadsTemplate 😄 template link
ProcessedExpressionTemplate 😄 template link
ProcessedVariantCallsTemplate 😄 template link
ProteomicsAssayTemplate 😄 template link
ProtocolTemplate 😄 template link
RNASeqTemplate 😄 template link
ScRNASeqTemplate 😄 template link
UpdateMilestoneReport 😄 template link
WESTemplate 😄 template link
WGSTemplate 😄 template link

Manifest Validation

manifest result expectation
GenomicsAssayTemplate_0.csv 😄 Lists can be blank if attr not required using ‘list like’ rule
GenomicsAssayTemplate_1.csv 😄 Mixing blanks and regular list values works
GenomicsAssayTemplate_2.csv 😄 Conditional validation for attributes is currently not supported
GenomicsAssayTemplate_control.csv 😄 There should be no issue with this template.
ScRNASeqTemplate_0.csv 😄 Single list val works by using ‘list like’ rule
ScRNASeqTemplate_1.csv 😄 Fail because of missing data in required field libraryStrand

Manifest Submission

manifest pass
GenomicsAssayTemplate_0.csv
GenomicsAssayTemplate_1.csv
GenomicsAssayTemplate_2.csv
GenomicsAssayTemplate_control.csv
ScRNASeqTemplate_0.csv

added peakCallingAlgorithms for ChIP-Seq
removed the comment for peakcallingalgorithm
Copy link
Contributor Author

@changtotheintothemoon changtotheintothemoon left a comment

Choose a reason for hiding this comment

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

LGTM

is_a: EpigenomicsAssayTemplate
description: Template for describing raw data from ChIP-Seq assays, typically used to identify DNA-protein interactions.
slots:
- antibodyID
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we make this antibodyID slot required, would this also effect other templates that might been using the slot?

@anngvu
Copy link
Collaborator

anngvu commented Nov 14, 2024

@changtotheintothemoon Once this template is considered "production-ready", to surface this in the app, you should add the template id and display name to the list here: https://github.com/nf-osi/nf-metadata-dictionary/blob/main/dca-template-config.json

added ChIP-SeqTemplate to the DCA App
changtotheintothemoon

This comment was marked as duplicate.

changtotheintothemoon

This comment was marked as duplicate.

This was linked to issues Nov 15, 2024
@anngvu
Copy link
Collaborator

anngvu commented Nov 15, 2024

@changtotheintothemoon Thanks! I think you're all done except one small issue. So in this review I discovered we have a typo for "EpigenomiscAssayTemplate" 😅 in the existing classes (#550), which is why inheritance is not working in the rendered preview link the tests above -- your version is correct. If you fix it, everything should be good for a 2-for-1 deal! (Please update dca-template-config again as well to reference the corrected class ids)

replaced "EpigenomiscAssayTemplate" to
"EpigenomicsAssayTemplate"
@anngvu anngvu self-requested a review November 18, 2024 15:53
@anngvu anngvu merged commit 4643e09 into main Nov 18, 2024
@anngvu anngvu deleted the add_chip_seq_assay branch November 18, 2024 15:54
@anngvu anngvu mentioned this pull request Nov 19, 2024
4 tasks
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.

Fix existing typo in class name Add template for ChIP-seq assay
3 participants