search

robmaz / distmap

Sequence alignment on Hadoop
0 stars 1 forks source link

readtools --trimmer support #53

Closed robmaz closed 6 years ago

robmaz commented 6 years ago

Daniel, could you maybe find the time to provide the new trimmer syntax in the appropriate places (= the couple lines in distmap where --trim-args is handled and $trim_args is set) with the current standard args as the default? The idea is to simply pass $trimming_flag (which may be empty, or the whole --trimmer ... arg list) to readtools, which itself is set from $trim_args unless --no-trim is given.

magicDGS commented 6 years ago

I was thinking to do something like that soon, creating a new helper-class for ReadTools related stuff. I will have a look later today.

robmaz commented 6 years ago

I don't think we need a helper class, just set $trim_args to a reasonable string, which will be passed through to readtools on uploading.

2018-02-20 16:13 GMT+01:00 Daniel Gómez-Sánchez notifications@github.com:

I was thinking to do something like that soon, creating a new helper-class for ReadTools related stuff. I will have a look later today.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/robmaz/distmap/issues/53#issuecomment-367008912, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad_FfKDo3F-vKDiC-FCXaffiQ3q6Udssks5tWuEwgaJpZM4SMEQQ .

magicDGS commented 6 years ago

I think that a helper class is a better approach for several reasons:

If you don't mind, I will propose the helper class and you can tell me if it is cleaner or not.

By the way, are all the parts related with it tracked in #54? That would be easier for me to develop this...

robmaz commented 6 years ago

Well, I hope I found them all ... some lines in distmap related to $trim_args and the upload commands in DataProcess.pm

2018-02-20 17:49 GMT+01:00 Daniel Gómez-Sánchez notifications@github.com:

I think that a helper class is a better approach for several reasons:

  • ReadTools is used in several places for distmap, and thus track the changes is difficult. This will allow to extract all related code to the class and track differences.
  • Integration changes will be easier to develop from my side and your side.
  • Modularity is important. In the case that ReadTools isn't required at some point, the whole class can be removed. If ReadTools is substituted by other program, a new helper class with similar methods can be implemented and the module refactor easily.
  • Easier to test if ReadTools changes are affecting the whole functionality by changing just the important part of the code.

If you don't mind, I will propose the helper class and you can tell me if it is cleaner or not.

By the way, are all the parts related with it tracked in #54 https://github.com/robmaz/distmap/pull/54? That would be easier for me to develop this...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/robmaz/distmap/issues/53#issuecomment-367041261, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad_FfF7xlo9w09vHYuBuKmEBo-sjJRRMks5tWveogaJpZM4SMEQQ .

robmaz commented 6 years ago

But if you mean the whole readtools integration, there is also DataDownloadAndMerge.pm to consider.

2018-02-20 17:53 GMT+01:00 Rupert Mazzucco rupert.mazzucco@gmail.com:

Well, I hope I found them all ... some lines in distmap related to $trim_args and the upload commands in DataProcess.pm

2018-02-20 17:49 GMT+01:00 Daniel Gómez-Sánchez notifications@github.com :

I think that a helper class is a better approach for several reasons:

  • ReadTools is used in several places for distmap, and thus track the changes is difficult. This will allow to extract all related code to the class and track differences.
  • Integration changes will be easier to develop from my side and your side.
  • Modularity is important. In the case that ReadTools isn't required at some point, the whole class can be removed. If ReadTools is substituted by other program, a new helper class with similar methods can be implemented and the module refactor easily.
  • Easier to test if ReadTools changes are affecting the whole functionality by changing just the important part of the code.

If you don't mind, I will propose the helper class and you can tell me if it is cleaner or not.

By the way, are all the parts related with it tracked in #54 https://github.com/robmaz/distmap/pull/54? That would be easier for me to develop this...

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/robmaz/distmap/issues/53#issuecomment-367041261, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad_FfF7xlo9w09vHYuBuKmEBo-sjJRRMks5tWveogaJpZM4SMEQQ .

magicDGS commented 6 years ago

I would add the trimming part because there are higher priority and part of the cleanup (apart of the version check); the rest of parts where ReadTools is handled would be part of a different PR (I guess).

robmaz commented 6 years ago

Good idea, getting the trimming to work is important to release this thing.

2018-02-20 17:56 GMT+01:00 Daniel Gómez-Sánchez notifications@github.com:

I would add the trimming part because there are higher priority and part of the cleanup (apart of the version check); the rest of parts where ReadTools is handled would be part of a different PR (I guess).

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/robmaz/distmap/issues/53#issuecomment-367043385, or mute the thread https://github.com/notifications/unsubscribe-auth/Ad_FfKbej_64QciBt_GmCO3lX8mLHBsWks5tWvksgaJpZM4SMEQQ .