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.

Sign up for GitHub

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

Jump to bottom

[FORMATS-114] Add processing description record. #134

Merged
merged 2 commits into from
May 13, 2017
Merged

[FORMATS-114] Add processing description record. #134

merged 2 commits into from
May 13, 2017

Conversation

fnothaft
Copy link
Member

Resolves #114.

@fnothaft fnothaft added this to the 0.11.0 milestone May 12, 2017
@fnothaft fnothaft requested a review from heuermh May 12, 2017 07:41
@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/bdg-formats-prb/155/
Test PASSed.

@fnothaft
Copy link
Member Author

I'd like to hold on merging this until bigdatagenomics/adam#1520 is in decent shape, but please start reviewing.

heuermh
heuermh requested changes May 12, 2017
View reviewed changes
src/main/resources/avro/bdg.avdl Outdated
@@ -87,6 +87,47 @@ record RecordGroupMetadata {
union { null, int } predictedMedianInsertSize = null;
union { null, string } platform = null;
union { null, string } platformUnit = null;

/**
The IDs of the processing steps that have been applied to this record group/
Copy link
Member

Choose a reason for hiding this comment

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

record group/ → record group.

src/main/resources/avro/bdg.avdl Outdated
/**
The IDs of the processing steps that have been applied to this record group/
*/
array<string> processingStepIds = [];
Copy link
Member

Choose a reason for hiding this comment

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

Since this *..* association is between RecordGroupMetadata, ProcessingStep (, and later Sample), all of which will not be large in number, why not have a direct reference instead of the id?

array<ProcessingStep> processingSteps = [];

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was on the hedge about this and could've gone either way. Since you prefer it this way, I'll change it to array<ProcessingStep>.

src/main/resources/avro/bdg.avdl Outdated
@@ -87,6 +87,47 @@ record RecordGroupMetadata {
union { null, int } predictedMedianInsertSize = null;
union { null, string } platform = null;
union { null, string } platformUnit = null;

/**
The IDs of the processing steps that have been applied to this record group/
Copy link
Member

Choose a reason for hiding this comment

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

And as long as we're in here, please consider #137

@fnothaft fnothaft force-pushed the issues/114-program-record branch from 3297887 to ccbfe5a Compare May 12, 2017 15:36
@fnothaft
Copy link
Member Author

Pushed fixed commits.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/bdg-formats-prb/157/
Test PASSed.

@heuermh heuermh merged commit b75e6ba into bigdatagenomics:master May 13, 2017
@heuermh
Copy link
Member

heuermh commented May 13, 2017

Thank you, @fnothaft

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heuermh heuermh heuermh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@fnothaft @AmplabJenkins @heuermh