NVIDIA-Genomics-Research / GenomeWorks

SDK for GPU accelerated genome assembly and analysis
https://clara-parabricks.github.io/GenomeWorks/
Apache License 2.0
286 stars 76 forks source link

[cudamapper] Let create_index() accept an IndexDescriptor #549

Closed nvvishanthi closed 4 years ago

nvvishanthi commented 4 years ago

As discussed in !536, allow the create_index() function to accept an IndexDescriptor object

closes !539

nvvishanthi commented 4 years ago

Currently this MR needs !536 before it can be merged.

Also, added a new create_index() function instead of replacing the old one, in case we needed to retain compatibility. Not sure if this is required, however

mimaric commented 4 years ago

I think we should completely replace the old interface, passing IndexDescriptor instead of read_id and number_of_reads makes way more sense. This does mean that a larger change is going to be needed, hopefully it will only consist of finding and replacing calls.

nvvishanthi commented 4 years ago

@mimaric I removed the old interface and fixed usage. Anything else that needs to be done?

mimaric commented 4 years ago

@mimaric I removed the old interface and fixed usage. Anything else that needs to be done?

I think this change should be propagated to IndexGPU as well, i.e. IndexGPU's constructor should take IndexDescriptor. That way it would be consistent with create_index() and as we already discussed IndexDescriptor makes the interface clearer

nvvishanthi commented 4 years ago

I think this change should be propagated to IndexGPU as well, i.e. IndexGPU's constructor should take IndexDescriptor.

Makes sense I will make the change. At first I didn't think to change that because IndexGPU is an internal type, and IndexDescriptor was an "interface" type, but I guess I was overthinking that

nvvishanthi commented 4 years ago

Done (I think) and all tests passed locally :). Had to change some of the tests, so I wanted to check and make sure I didn't alter the behavior to something incorrect

mimaric commented 4 years ago

Ok, so ready for re-review?

nvvishanthi commented 4 years ago

yup I think so