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

Cram code base #9

Closed
wants to merge 51 commits into from
Closed

Cram code base #9

wants to merge 51 commits into from

Conversation

vadimzalunin
Copy link
Contributor

Just rebased the branch from master. Briefly:

  • adds new htsjdk.samtools.cram package that contains CRAM backend like codecs, structures and thins
  • adds CRAM type in SamReader
  • CRAMFileReader and writer implementations
  • changed visibility of TagValueAndUnsignedArrayFlag, used in the cram package

@mp15
Copy link
Member

mp15 commented Jun 2, 2014

I notice all the new work is licensed under the Apache License 2.0 whilst the rest of the project is licensed under MIT. I assume this is intended?

@vadimzalunin
Copy link
Contributor Author

Historical reasons -- cramtools is under AL2.
Personally I am +0 on AL2 but the only difference I can see is patent grant. That is AL2 explicitly grants rights to the patents used whereas MIT does not say it.

Does anyone have an opinion on AL2 vs MIT in hts-jdk? The default is MIT I guess.

@tfenne
Copy link
Member

tfenne commented Jun 2, 2014

I'm fine with both licenses. In theory if we changed to ASL2 I should go back through the technology licensing bureaucracy at Broad and get that approved, which is the only downside.

@vadimzalunin
Copy link
Contributor Author

Hold on, I'll check with our bureaucracy :)

bradtaylor added 9 commits June 4, 2014 17:56
These changes were from before the Picard-public/htsjdk split. This branch is synchronous with the branch of the same name in Picard-public
Still lots of work to do. Just a commit before going home on 5/29
Alterations to implement Queue interface complete
pending unit testing and usage (likely will find bugs)
to reflect previous change in DiskBackedQueue to implement Queue,
store file records a little more simply
DiskBackedQueue now appropriately implements a queue. Added
improvements and additional cases to the unit test. Unit tests
pass.

Added a TemporaryDuplicateMarkedFlag to SAMRecord. This is set
temporarily during Picard-public MarkDuplicatesWithMateCigar to
indicate that a record is available for emission from the algorithm.
For use with in-memory records that are never spilled to disk.
Messing with SAMRecord was a bad design choice.
<natures>
<nature>org.eclipse.jdt.core.javanature</nature>
</natures>
<name>cramone</name>
Copy link
Member

Choose a reason for hiding this comment

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

htsjdk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed .classpath and .project as these are Eclipse files


private Sam2CramRecordFactory sam2CramRecordFactory;
private OutputStream os;

Copy link
Contributor

Choose a reason for hiding this comment

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

fileName and os are never initialized. When a new CRAMFileWriter is instantiated all subsequent calls with return NPEs as a result. There should probably be a constructor that can take a File or an OutputStream similar to BAMFileWriter.

multiple fixes in CRAM writer to make it actually work.
CRAMFileWriter: proper initialization of output stream and record
factory;
Slice: write empty MD5 if not set;
CRAMIterator: nextContainer clears records so that hasNext returns
false.
fixed missing pairing in CRAMFileWriter;
fixed header written twice;
added CRAM reader to SamFileReader make method;
added tests to .classpath
added testng output to .gitingore;
tested with CramFileWriterTest.
@nh13
Copy link
Member

nh13 commented Oct 16, 2014

@vadimzalunin have we fixed the secondary read mate info issue?

@vadimzalunin
Copy link
Contributor Author

I think so, although I have not tested it as I'm on ASHG with somewhat
limited possibilities. I'm back to office on Wednesday but maybe someone
could run a quick test before that?

On Thu, Oct 16, 2014 at 3:23 AM, Nils Homer [email protected]
wrote:

@vadimzalunin https://github.com/vadimzalunin have we fixed the
secondary read mate info issue?


Reply to this email directly or view it on GitHub
#9 (comment).

@jacarey
Copy link
Contributor

jacarey commented Dec 12, 2014

I'm going to close this pull request. The cram code base was merged in to master after a rebase and the SamReader changes needed to take advantage of the cram code base. There is one change that I need to cherry pick from this branch to bring master up to date. After that I'll delete this branch and open a few issues for @vadimzalunin.

@jacarey jacarey closed this Dec 12, 2014
@droazen droazen deleted the cram_code_base branch October 22, 2015 23:17
cmnbroad pushed a commit that referenced this pull request Nov 8, 2018
* Adding MAINTAINERS.md file with information about the current maintainers.  Please keep this up to date.
jmthibault79 added a commit that referenced this pull request Mar 26, 2019
# This is the 1st commit message:

move Container.setByteOffset() call inside ContainerIO.readContainer()
- also ContainerHeaderIO.readContainerHeader()

restrict access to setByteOffset() and encapsulate Container.slices

# This is the commit message #2:

oops

# This is the commit message #3:

a little unrelated cleanup

# This is the commit message #4:

oops

# This is the commit message #5:

better CRAIEntryTest

# This is the commit message #6:

test improvements and a fix

# This is the commit message #7:

comment

# This is the commit message #8:

javadoc

# This is the commit message #9:

review comments

# This is the commit message #10:

comments and clarification for CRAMBAIIndexer
yfarjoun pushed a commit to yfarjoun/htsjdk that referenced this pull request Jun 5, 2019
Fixes samtools#9.  Also add footnote referencing the GZIP RFC.
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.

5 participants