Repository: open2c/bioframe

Issue:
select recently changed behavior causing issues in cooltools
as described in https://github.com/open2c/cooltools/issues/435 :

bioframe select became more inclusive and is selecting "more" with v0.4.0 ...
with bioframe 0.3.1 
``` python
In [16]: bioframe.select(track, ("chr1",100000000,150000000)).shape
Out[16]: (50, 4)
```
and with bioframe 0.4.0:
```python
In [4]: bioframe.select(track, ("chr1",100000000,150000000)).shape
Out[4]: (51, 4)
```

sample output of the `bioframe.select` here:
![image](https://user-images.githubusercontent.com/6790270/232927465-f9bf3ef6-8aaa-479c-979a-3d6cabb43f07.png)
that *99th* bins is not included with the older bioframe

should cooltools adapt to this change or will there be an option to do `inclusive/exclusive` selection ? like `inclusive_left`, `inclusive_right` ?

Discussion:
nvictus: Added various tests, but could still use a bit more exploration of potential edge cases we've missed or inconsistencies.
gfudenberg: funky!
was this a common use case for you?

On Fri, Apr 21, 2023 at 4:25 PM Nezar Abdennur ***@***.***>
wrote:

> ***@***.**** commented on this pull request.
> ------------------------------
>
> In bioframe/ops.py
> <https://github.com/open2c/bioframe/pull/147#discussion_r1174222181>:
>
> >          mask = df[ck] == chrom
> +    else:
> +        if end is None:
>
> Yup, bioframe.parse_region now accepts unspecified ends like "chr2:100-"
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/open2c/bioframe/pull/147#discussion_r1174222181>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEV7GZME6TXC3EHJFLIQHOLXCMJQNANCNFSM6AAAAAAXGCYA5M>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>

nvictus: It figured it was annoying to always have to look up a chromsizes to specify an `end` for bioframe. And it aligns with Python slice semantics: `x[3:] == x[slice(3, None)]` --> `parse_region("chrX:3-") == ('chrX', 3, None)`
gfudenberg: Gotcha.
Should (is?)

chrX:-3 supported? I.e. ('chrX',none,3)

On Fri, Apr 21, 2023 at 4:35 PM Nezar Abdennur ***@***.***>
wrote:

> It figured it was annoying to always have to look up a chromsizes to
> specify an end for bioframe. And it aligns with Python slice semantics: x[3:]
> == x[slice(3, None)] --> parse_region("chrX:3-") == ('chrX', 3, None)
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/open2c/bioframe/pull/147#issuecomment-1518423110>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEV7GZPGLHMJP4IC2WLLQVLXCMKV5ANCNFSM6AAAAAAXGCYA5M>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>

nvictus: Nope. It's messy because it looks like a negative coordinate and it's easy enough to do `chrX:0-3` because the start is known.
gfudenberg: right, of course!

On Fri, Apr 21, 2023 at 4:39 PM Nezar Abdennur ***@***.***>
wrote:

> Nope. It's messy because it looks like a negative coordinate and it's easy
> enough to do chrX:0-3 because the start is known.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/open2c/bioframe/pull/147#issuecomment-1518424278>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEV7GZMYSUTHVKAPIRRTHC3XCMLCXANCNFSM6AAAAAAXGCYA5M>
> .
> You are receiving this because your review was requested.Message ID:
> ***@***.***>
>


Patch:
diff --git a/bioframe/ops.py b/bioframe/ops.py
index 2da92b6..4503114 100644
--- a/bioframe/ops.py
+++ b/bioframe/ops.py
@@ -54,12 +54,19 @@ def select_mask(df, region, cols=None):
     _verify_columns(df, [ck, sk, ek])
 
     chrom, start, end = parse_region(region)
+
     if chrom is None:
         raise ValueError("no chromosome detected, check region input")
-    if (start is not None) and (end is not None):
-        mask = (df[ck] == chrom) & (df[sk] < end) & (df[ek] >= start)
-    else:
+
+    if start is None:
         mask = df[ck] == chrom
+    else:
+        if end is None:
+            end = np.inf
+        mask = (df[ck] == chrom) & (
+            ((df[sk] < end) & (df[ek] > start)) | 
+            ((df[sk] == df[ek]) & (df[sk] == start))  # include points at query start
+        )
     return mask.to_numpy()
 
 


Test patch:
diff --git a/tests/test_ops.py b/tests/test_ops.py
index 742ae64..7d83165 100644
--- a/tests/test_ops.py
+++ b/tests/test_ops.py
@@ -64,114 +64,6 @@ def mock_bioframe(num_entries=100):
 
 
 ############# tests #####################
-def test_select_mask_indices_labels():
-    df1 = pd.DataFrame(
-        [["chrX", 3, 8], ["chr1", 4, 5], ["chrX", 1, 5]],
-        columns=["chrom", "start", "end"],
-    )
-    region1 = "chr1:4-10"
-    df_result = pd.DataFrame([["chr1", 4, 5]], columns=["chrom", "start", "end"])
-    mask = bioframe.select_mask(df1, region1)
-    pd.testing.assert_frame_equal(
-        df_result, df1.loc[mask].reset_index(drop=True)
-    )
-    labels = bioframe.select_labels(df1, region1)
-    pd.testing.assert_frame_equal(
-        df_result, df1.loc[labels].reset_index(drop=True)
-    )
-    idx = bioframe.select_indices(df1, region1)
-    pd.testing.assert_frame_equal(
-        df_result, df1.iloc[idx].reset_index(drop=True)
-    )
-
-    df2 = pd.DataFrame(
-        [["chrX", 3], ["chr1", 4], ["chrX", 1]],
-        columns=["chrom", "pos"],
-    )
-    region2 = "chr1:4-10"
-    df_result = pd.DataFrame([["chr1", 4]], columns=["chrom", "pos"])
-    mask = bioframe.select_mask(df2, region2, cols=["chrom", "pos", "pos"])
-    pd.testing.assert_frame_equal(
-        df_result, df2.loc[mask].reset_index(drop=True)
-    )
-    labels = bioframe.select_labels(df2, region2, cols=["chrom", "pos", "pos"])
-    pd.testing.assert_frame_equal(
-        df_result, df2.loc[labels].reset_index(drop=True)
-    )
-    idx = bioframe.select_indices(df2, region2, cols=["chrom", "pos", "pos"])
-    pd.testing.assert_frame_equal(
-        df_result, df2.iloc[idx].reset_index(drop=True)
-    )
-
-
-def test_select():
-    df1 = pd.DataFrame(
-        [["chrX", 3, 8], ["chr1", 4, 5], ["chrX", 1, 5]],
-        columns=["chrom", "start", "end"],
-    )
-
-    region1 = "chr1:4-10"
-    df_result = pd.DataFrame([["chr1", 4, 5]], columns=["chrom", "start", "end"])
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df1, region1).reset_index(drop=True)
-    )
-
-    region1 = "chrX"
-    df_result = pd.DataFrame(
-        [["chrX", 3, 8], ["chrX", 1, 5]], columns=["chrom", "start", "end"]
-    )
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df1, region1).reset_index(drop=True)
-    )
-
-    region1 = "chrX:4-6"
-    df_result = pd.DataFrame(
-        [["chrX", 3, 8], ["chrX", 1, 5]], columns=["chrom", "start", "end"]
-    )
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df1, region1).reset_index(drop=True)
-    )
-
-    ### select with non-standard column names
-    region1 = "chrX:4-6"
-    new_names = ["chr", "chrstart", "chrend"]
-    df1 = pd.DataFrame(
-        [["chrX", 3, 8], ["chr1", 4, 5], ["chrX", 1, 5]],
-        columns=new_names,
-    )
-    df_result = pd.DataFrame(
-        [["chrX", 3, 8], ["chrX", 1, 5]],
-        columns=new_names,
-    )
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df1, region1, cols=new_names).reset_index(drop=True)
-    )
-    region1 = "chrX"
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df1, region1, cols=new_names).reset_index(drop=True)
-    )
-
-    ### select from a DataFrame with NaNs
-    colnames = ["chrom", "start", "end", "view_region"]
-    df = pd.DataFrame(
-        [
-            ["chr1", -6, 12, "chr1p"],
-            [pd.NA, pd.NA, pd.NA, "chr1q"],
-            ["chrX", 1, 8, "chrX_0"],
-        ],
-        columns=colnames,
-    ).astype({"start": pd.Int64Dtype(), "end": pd.Int64Dtype()})
-    df_result = pd.DataFrame(
-        [["chr1", -6, 12, "chr1p"]],
-        columns=colnames,
-    ).astype({"start": pd.Int64Dtype(), "end": pd.Int64Dtype()})
-
-    region1 = "chr1:0-1"
-    pd.testing.assert_frame_equal(
-        df_result, bioframe.select(df, region1).reset_index(drop=True)
-    )
-
-
 def test_trim():
 
     ### trim with view_df
diff --git a/tests/test_ops_select.py b/tests/test_ops_select.py
new file mode 100644
index 0000000..2bee034
--- /dev/null
+++ b/tests/test_ops_select.py
@@ -0,0 +1,230 @@
+import pandas as pd
+import numpy as np
+import pytest
+
+import bioframe
+
+
+def test_select():
+    df = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chr1", 4, 5], 
+         ["chrX", 1, 5]],
+        columns=["chrom", "start", "end"],
+    )
+
+    result = pd.DataFrame(
+        [["chr1", 4, 5]], 
+        columns=["chrom", "start", "end"]
+    )
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chr1:4-10").reset_index(drop=True)
+    )
+
+    result = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chrX", 1, 5]], 
+         columns=["chrom", "start", "end"]
+    )
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chrX").reset_index(drop=True)
+    )
+
+    result = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chrX", 1, 5]], 
+         columns=["chrom", "start", "end"]
+    )
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chrX:4-6").reset_index(drop=True)
+    )
+
+    # Query range not in the dataframe
+    assert len(bioframe.select(df, "chrZ")) == 0
+    assert len(bioframe.select(df, "chr1:100-1000")) == 0
+    assert len(bioframe.select(df, "chr1:1-3")) == 0
+
+    # Invalid query range
+    with pytest.raises(ValueError):
+        bioframe.select(df, "chr1:1-0")
+
+
+def test_select__with_colnames():
+    ### select with non-standard column names
+    new_names = ["chr", "chrstart", "chrend"]
+    df = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chr1", 4, 5], 
+         ["chrX", 1, 5]],
+        columns=new_names,
+    )
+    result = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chrX", 1, 5]],
+        columns=new_names,
+    )
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chrX:4-6", cols=new_names).reset_index(drop=True)
+    )
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chrX", cols=new_names).reset_index(drop=True)
+    )
+
+
+def test_select__with_nulls():
+    ### select from a DataFrame with NaNs
+    colnames = ["chrom", "start", "end", "view_region"]
+    df = pd.DataFrame(
+        [
+            ["chr1", -6, 12, "chr1p"],
+            [pd.NA, pd.NA, pd.NA, "chr1q"],
+            ["chrX", 1, 8, "chrX_0"],
+        ],
+        columns=colnames,
+    ).astype({"start": pd.Int64Dtype(), "end": pd.Int64Dtype()})
+    
+    result = pd.DataFrame(
+        [["chr1", -6, 12, "chr1p"]],
+        columns=colnames,
+    ).astype({"start": pd.Int64Dtype(), "end": pd.Int64Dtype()})
+
+    pd.testing.assert_frame_equal(
+        result, bioframe.select(df, "chr1:0-1").reset_index(drop=True)
+    )
+
+
+def test_select__mask_indices_labels():
+    df = pd.DataFrame(
+        [["chrX", 3, 8], 
+         ["chr1", 4, 5], 
+         ["chrX", 1, 5]],
+        columns=["chrom", "start", "end"],
+    )
+    
+    region = "chr1:4-10"
+    answer = pd.DataFrame(
+        [["chr1", 4, 5]], 
+        columns=["chrom", "start", "end"]
+    )
+    
+    result = bioframe.select(df, region)
+    pd.testing.assert_frame_equal(
+        answer, result.reset_index(drop=True)
+    )
+    mask = bioframe.select_mask(df, region)
+    pd.testing.assert_frame_equal(
+        answer, df.loc[mask].reset_index(drop=True)
+    )
+    labels = bioframe.select_labels(df, region)
+    pd.testing.assert_frame_equal(
+        answer, df.loc[labels].reset_index(drop=True)
+    )
+    idx = bioframe.select_indices(df, region)
+    pd.testing.assert_frame_equal(
+        answer, df.iloc[idx].reset_index(drop=True)
+    )
+
+
+def test_select__query_intervals_are_half_open():
+    df = pd.DataFrame({
+        "chrom": ["chr1", "chr1", 
+                  "chr2", "chr2", "chr2", "chr2", "chr2", "chr2"],
+        "start": [0, 10, 
+                  10, 20, 30, 40, 50, 60],
+        "end":  [10, 20, 
+                 20, 30, 40, 50, 60, 70],
+        "name": ["a", "b", 
+                 "A", "B", "C", "D", "E", "F"],
+    })
+
+    result = bioframe.select(df, "chr1")
+    assert (result["name"] == ["a", "b"]).all()
+
+    result = bioframe.select(df, "chr2:20-70")
+    assert (result["name"] == ["B", "C", "D", "E", "F"]).all()
+
+    result = bioframe.select(df, "chr2:20-75")
+    assert (result["name"] == ["B", "C", "D", "E", "F"]).all()
+
+    result = bioframe.select(df, "chr2:20-")
+    assert (result.index == [3, 4, 5, 6, 7]).all()
+
+    result = bioframe.select(df, "chr2:20-30")
+    assert (result["name"] == ["B"]).all()
+
+    result = bioframe.select(df, "chr2:20-40")
+    assert (result["name"] == ["B", "C"]).all()
+
+    result = bioframe.select(df, "chr2:20-45")
+    assert (result["name"] == ["B", "C", "D"]).all()
+
+    result = bioframe.select(df, "chr2:19-45")
+    assert (result["name"] == ["A", "B", "C", "D"]).all()
+
+    result = bioframe.select(df, "chr2:25-45")
+    assert (result["name"] == ["B", "C", "D"]).all()
+
+    result = bioframe.select(df, "chr2:25-50")
+    assert (result["name"] == ["B", "C", "D"]).all()
+
+    result = bioframe.select(df, "chr2:25-51")
+    assert (result["name"] == ["B", "C", "D", "E"]).all()
+
+
+def test_select__with_point_intervals():
+    # Dataframe containing "point intervals"
+    df = pd.DataFrame({
+        "chrom": ["chr1", "chr1", 
+                  "chr2", "chr2", "chr2", "chr2", "chr2", "chr2"],
+        "start": [0, 10, 
+                  10, 20, 30, 40, 50, 60],
+        "end":  [10, 10, 
+                 20, 30, 40, 50, 50, 70],
+        "name": ["a", "b", 
+                 "A", "B", "C", "D", "E", "F"],
+    })
+    result = bioframe.select(df, "chr1")
+    assert (result["name"] == ["a", "b"]).all()
+
+    result = bioframe.select(df, "chr1:4-10")
+    assert (result["name"] == ["a"]).all()
+
+    result = bioframe.select(df, "chr1:4-4")
+    assert (result["name"] == ["a"]).all()
+
+    result = bioframe.select(df, "chr1:10-15")
+    assert (result["name"] == ["b"]).all()
+
+    result = bioframe.select(df, "chr2:20-70")
+    assert (result["name"] == ["B", "C", "D", "E", "F"]).all()
+
+    result = bioframe.select(df, "chr2:49-70")
+    assert (result["name"] == ["D", "E", "F"]).all()
+
+    result = bioframe.select(df, "chr2:50-70")
+    assert (result["name"] == ["E", "F"]).all()
+
+    result = bioframe.select(df, "chr2:50-51")
+    assert (result["name"] == ["E"]).all()
+
+    result = bioframe.select(df, "chr2:50-50")
+    assert (result["name"] == ["E"]).all()
+
+
+def test_select__with_points():
+    # Dataframe of points
+    df = pd.DataFrame(
+        [["chrX", 3, "A"], 
+         ["chr1", 4, "C"], 
+         ["chrX", 1, "B"]],
+        columns=["chrom", "pos", "name"],
+    )
+    
+    result = bioframe.select(df, "chr1:4-10", cols=["chrom", "pos", "pos"])
+    assert (result["name"] == ["C"]).all()
+
+    result = bioframe.select(df, "chr1:3-10", cols=["chrom", "pos", "pos"])
+    assert (result["name"] == ["C"]).all()
+
+    result = bioframe.select(df, "chr1:4-4", cols=["chrom", "pos", "pos"])
+    assert (result["name"] == ["C"]).all()