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

Keep OverlapDetector generic but normalize the getters for BedRecord and Interval #36

Merged
merged 4 commits into from
Aug 1, 2024

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Jul 17, 2024

Alternate to #34

pybedlite/bed_record.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 95.40230% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.66%. Comparing base (2d71949) to head (cf99c00).

Files Patch % Lines
pybedlite/bed_record.py 66.66% 2 Missing ⚠️
pybedlite/tests/test_overlap_detector.py 96.55% 2 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           cv_generic_overlap_detector      #36      +/-   ##
===============================================================
+ Coverage                        92.61%   92.66%   +0.05%     
===============================================================
  Files                                8        8              
  Lines                              609      641      +32     
  Branches                           110      116       +6     
===============================================================
+ Hits                               564      594      +30     
- Misses                              27       30       +3     
+ Partials                            18       17       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@clintval clintval self-assigned this Jul 17, 2024
@clintval clintval self-requested a review July 17, 2024 14:35
@nh13 nh13 marked this pull request as ready for review July 19, 2024 18:21
@nh13 nh13 changed the title alternate Alternate to #34 Jul 19, 2024
Copy link
Member

@clintval clintval left a comment

Choose a reason for hiding this comment

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

I like it but I would only ask we add property names to make the interface between BedRecord and Interval consistent and not add the zero_based_start and zero_based_open_end at this time.

@clintval clintval removed their assignment Jul 30, 2024
@nh13 nh13 temporarily deployed to github-action-ci July 30, 2024 23:53 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-action-ci July 30, 2024 23:53 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-action-ci July 30, 2024 23:53 — with GitHub Actions Inactive
@nh13 nh13 requested a review from clintval July 30, 2024 23:53
@nh13 nh13 temporarily deployed to github-action-ci July 30, 2024 23:53 — with GitHub Actions Inactive
pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
generic type contained within the :class:`~pybedlite.overlap_detector.OverlapDetector`.
"""


class OverlapDetector(Generic[_GenericGenomicSpan], Iterable[_GenericGenomicSpan]):
class OverlapDetector(Generic[GenericGenomicsSpan], Iterable[GenericGenomicsSpan]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is GenericGenomicSpan necessary? StrandedGenomicSpan subclasses GenomicSpan, why not just use GenomicSpan here?

Copy link
Member

@clintval clintval Jul 31, 2024

Choose a reason for hiding this comment

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

I kind of like it because it is explicit for the reader and shouldn't have any performance impact. Otherwise it might be overlooked that there is a tiny part of the OverlapDetector that does change behavior based on strand:

@staticmethod
def _is_negative(interval: GenomicSpan) -> bool:
return getattr(interval, "is_negative", False)

self._is_negative(intv),

Copy link
Member

@clintval clintval left a comment

Choose a reason for hiding this comment

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

What do you think about adding a negative property to BedRecord so we have fewer duplicated getters across both classes instead of introducing is_negative to both?

pybedlite/bed_record.py Outdated Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Show resolved Hide resolved
pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
generic type contained within the :class:`~pybedlite.overlap_detector.OverlapDetector`.
"""


class OverlapDetector(Generic[_GenericGenomicSpan], Iterable[_GenericGenomicSpan]):
class OverlapDetector(Generic[GenericGenomicsSpan], Iterable[GenericGenomicsSpan]):
Copy link
Member

@clintval clintval Jul 31, 2024

Choose a reason for hiding this comment

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

I kind of like it because it is explicit for the reader and shouldn't have any performance impact. Otherwise it might be overlooked that there is a tiny part of the OverlapDetector that does change behavior based on strand:

@staticmethod
def _is_negative(interval: GenomicSpan) -> bool:
return getattr(interval, "is_negative", False)

self._is_negative(intv),

pybedlite/overlap_detector.py Outdated Show resolved Hide resolved
@nh13 nh13 force-pushed the nh_generic_overlap_detector branch from e209994 to cf99c00 Compare July 31, 2024 21:08
@nh13 nh13 temporarily deployed to github-action-ci July 31, 2024 21:08 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-action-ci July 31, 2024 21:08 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-action-ci July 31, 2024 21:08 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-action-ci July 31, 2024 21:08 — with GitHub Actions Inactive
@clintval clintval changed the title Alternate to #34 Keep OverlapDetector generic but normalize the getters for BedRecord and Interval Aug 1, 2024
@clintval clintval merged commit 13b7834 into cv_generic_overlap_detector Aug 1, 2024
6 checks passed
@clintval clintval deleted the nh_generic_overlap_detector branch August 1, 2024 13:17
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.

3 participants