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

BED GRAPH files support #13

Merged
merged 5 commits into from
Jul 25, 2017
Merged

BED GRAPH files support #13

merged 5 commits into from
Jul 25, 2017

Conversation

SilinPavel
Copy link
Member

@SilinPavel SilinPavel commented Jul 14, 2017

Description

Background

The bedGraph format is very popular format, but unfortunately currently NGB does not allow to visualize such files directly, only bigWig files are supported. This pull request directs to add the support of Bed graph files.

Changes

New classes for work with Bed graph files were added. WigManager was split to two classes for a decomposition and also BedGraph manager was invented as a child to AbstractWigManager.
FacadeWigManager currently get calls from WigController and delegate it to right implementation of AbstractWigManager.

Acceptance criteria

All tests must pass and also manually registration, viewing of BigWig and BedGraph files should be performed.

Check list

  • Unit tests are provided
  • Integration tests are provided (if CLI/API was changed)
  • Documentation is updated

@sidoruka
Copy link
Contributor

sidoruka commented Jul 15, 2017

@SilinPavel , thanks!
So can I register bedGraphs via CLI now? If so - Integration tests are provided (if CLI/API was changed) item shall be considered. Have you provided any integration tests for bedGraph registration?

Regarding Acceptance criteria section - could you please provide a full test scenario?

  1. Public bedGraph file to use in a scenario
  2. CLI commands that will register bedGraph
  3. Expected behavior of the GUI for that file:
  • Image for whole chr view
  • Image for 100bp range view

Once these issues are fixed, @mzueva and @vpolyakov88 could you please check this feature?

@SilinPavel
Copy link
Member Author

@sidoruka Thanks! b56e65c here is integration test cases for working with BedGraph files via CLI. Sorry for missing it up!
I also trying to find some open data set with bedGraph files, expected view of WIG tracks will follow soon!

@SilinPavel
Copy link
Member Author

SilinPavel commented Jul 17, 2017

@vpolyakov88 here are BedGraph and BIG WIG files that was created from:

ftp://ftp-trace.ncbi.nlm.nih.gov/giab/ftp/data/ChineseTrio/HG005_NA24631_son/OsloUniversityHospital_Exome/151002_7001448_0359_AC7F6GANXX_Sample_HG005-EEogPU_v02-KIT-Av5_CGCATACA_L008.posiSrt.markDup.bam 

https://goo.gl/Q7tFpo

This files contain information about coverage on a chr3. So you can check that values are the same. For example for me it looks like:

full chromosome view:
image

100 bp view:
image

@sidoruka
Copy link
Contributor

@SilinPavel , great!
@mzueva , @vpolyakov88 please proceed with the review.

@vpolyakov88
Copy link
Collaborator

vpolyakov88 commented Jul 18, 2017

@SilinPavel I've found a bug. In case when user zooms to scale 211bp and less in region where end of one region in the same time start of next region (first should be more) second region isn't displaying on attached data go to chr3: 62121 - 62361.
exp.: wig and bdg looks the same
act.: on bdg right region isn't displayed
server send same data for wig track and bedGraph track.
000

@SilinPavel
Copy link
Member Author

@vpolyakov88 Thank you a lot! There was the bug because of losing the last bedGraph feature in a query iterator. It was fixed in 3ddca48. Please check it one more time. And don't hesitate to report me about bugs if any!

@mzueva
Copy link
Collaborator

mzueva commented Jul 20, 2017

@SilinPavel New code and refactoring are quite good. Please fix year in the license header and check that all new classes has javadoc. I think that we should refactor hierarchy of AbstactWigManager and FacadeWigManager and use the approach from VcfManager and AbstractVcfReader (do not use autowired beans for stateless file readers).

@@ -222,7 +222,7 @@ ngb reg_file|rf [<REFERENCE_NAME>|<REFERENCE_ID>] [<PATH_TO_NGS_FILE>] [options]

Registers a specified file. At least two arguments have to be specified:
Previously registered reference sequence file from NGB server. Reference file can be addressed by name or an identifier
Flesystem path foor the file to be registered. BAM, VCF, GFF, GTF, BED, SEG files are accepted. GZipped files are also accepted in a format <FILE_NAME>.<FILE_EXT>.gz, e.g.: my_variants.vcf.gz.
Flesystem path foor the file to be registered. BAM, VCF, GFF, GTF, BED, SEG, WIG, BED GRAPH files are accepted. GZipped files are also accepted in a format <FILE_NAME>.<FILE_EXT>.gz, e.g.: my_variants.vcf.gz.
Copy link
Collaborator

Choose a reason for hiding this comment

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

foor -> to

/*
* MIT License
*
* Copyright (c) 2016 EPAM Systems
Copy link
Collaborator

Choose a reason for hiding this comment

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

2017 year

import java.util.stream.Collectors;

import static com.epam.catgenome.component.MessageHelper.getMessage;

Copy link
Collaborator

Choose a reason for hiding this comment

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

javadoc

/**
* Manages all work with BedGraph files.
* */
@Service
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need AbstractWigManager implementations as Spring beans. Task a look at VcfReader interface and it's implementations.

Copy link
Member Author

@SilinPavel SilinPavel Jul 20, 2017

Choose a reason for hiding this comment

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

@mzueva Thanks for the review. Could you clarify some points? Am I right if I think that you propose to autowire all beans that we need into FacadeWigManager and provide it to our AbstractWigManager implementations into their constructors? in this case all AbstractWigManager hierarchy won't be spring beans and will be created simply with constructors call?

@SilinPavel
Copy link
Member Author

@mzueva take a look one more time please! Now it is more pretty, but bear in mind that we still need to provide the fileManager and the biologicalDataItemManager to a AbstractWigProcessor realization due to we need register and get index into a BedGraphProcessor object.

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