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

Umi format explore fix #73

Merged

Conversation

alneberg
Copy link
Member

The methods len_after_index and len_before_index on the adaptor parts were not correct for index regions containing UMIs. This should fix it and also:

  • Rename the methods to indicate index_region instead of index
  • Remove the currently unused AdaptorPart.get_mask

@alneberg alneberg marked this pull request as draft February 13, 2024 09:47
@alneberg alneberg marked this pull request as ready for review February 13, 2024 13:08
# Dynamically assign attributes
self.umi = re.findall(udelim, self.sequence)

# TODO Duplicated from Adaptor class, will be merged later
Copy link
Member

@remiolsen remiolsen Feb 13, 2024

Choose a reason for hiding this comment

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

I was not happy with this part when I first wrote it, and am not happy with seeing for a second time. Problem might be I wrote it too generic. In reality it's only the first case that is valid, i.e. the INDEX+UMI configuration provided by that company whose name is mostly known by it's three letter abbreviation.

I'm leaning towards allowing this. As I stated in the previous PR, these two adaptor classes needs a proper cleaning up.
Just as a side-note, from purely semantics it's not clear to me what an "adaptorpart" is and what such an object would be a reflection of.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I wanted to support the same type as you did in the other code essentially.

For me each adaptor would have two adaptorparts, i.e. i7 and i5. Does that make sense?

Copy link
Member

@remiolsen remiolsen Feb 14, 2024

Choose a reason for hiding this comment

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

Don't worry, I'm being nitpicky over the name. It might have been more clear if AdaptorPart was a child class of Adaptor. But that's currently a little bit weird since AdaptorParts are instance variables of Adaptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

No worries, but no I think child classes would have been more confusing actually. Their just the two parts of an Adapter to DRY up all the methods and attributes which are identical between i7 and i5.

Copy link
Member

Choose a reason for hiding this comment

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

In isolation think it would be less confusing. BUT it doesn't really fit with how the adaptor class is currently used. So it would mean a whole lot of refactoring for the sake of idealism, which I don't advocate for.

@alneberg alneberg merged commit a82f3fc into NationalGenomicsInfrastructure:master Feb 14, 2024
6 checks passed
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.

2 participants