Stop using reference IDs and use reference names instead #217
Conversation
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/280/ |
| */ | ||
| recordNames.keys.filter(!dict.recordNames.contains(_)).foreach { | ||
| recordNames.keys.foreach { |
There was a problem hiding this comment.
It seems like we should be able to make this into a map/flatMap and to make val assign into an immutable data structure. Is there a reason it needs to be mutable?
Also, I'd prefer to change the name of the val assign to something different, as I feel that assign hash collides...
There was a problem hiding this comment.
I think we can just rely on the overloaded ++ method, since our merge semantics are the same as what Scala provides for Maps. What do you think?
There was a problem hiding this comment.
Good call! I think that'd be a clean implementation.
Neal notifications@github.com wrote:
*/
- recordNames.keys.filter(!dict.recordNames.contains(_)).foreach {
- recordNames.keys.foreach {
I think we can just rely on the overloaded ++ method, since our merge semantics are the same as what Scala provides for Maps. What do you think?
Reply to this email directly or view it on GitHub:
https://github.com/bigdatagenomics/adam/pull/217/files#r11774102In adam-core/src/main/scala/org/bdgenomics/adam/models/SequenceDictionary.scala:
> */ > - recordNames.keys.filter(!dict.recordNames.contains(_)).foreach { > + recordNames.keys.foreach {I think we can just rely on the overloaded ++ method, since our merge semantics are the same as what Scala provides for Maps. What do you think?
—
Reply to this email directly or view it on GitHub.
|
Thanks for putting this together @nealsid! This looks great; I had a few small comments inline. I look forward to the finishing tests, and having this merged in. |
|
Thanks for the comments, Frank! |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/282/ |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/283/ |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/284/ |
|
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/ADAM-prb/288/ |
|
All automated tests passed. |
|
Jenkins, test this please. |
|
All automated tests passed. |
|
Oops, actually hold off. I'll rebase & squash. |
|
Jenkins, test this place |
|
Jenkins, test this please. |
|
All automated tests passed. |
We decided that using RefSeq names for our contigs was better practice than minting our own IDs and handling complicated sequence dictionary merges.
|
All automated tests passed. |
|
This is good to go now. Thanks! |
Stop using reference IDs and use reference names instead
|
Merged! Thanks @nealsid! |
I have a few more tests to fix but this is definitely ready for more eyeballs on it. Hopefully I'll be able to fix the remaining ones tomorrow.