Skip to content

Conversation

@jorenretel
Copy link
Collaborator

@jorenretel jorenretel commented Apr 24, 2025

see comment: #17 (comment)

I can not replicate the behavior where there are zero's in the output matrix where there should not be.

  • I parameterised all the tests that test whether intervals_to_values cuda kernels are working as expected to also use cp.nan as default value. They are not indicating any errors.
  • I added an extra test to test against pyBigWig (not converting nans to zeros), included a genomic position in example_data/some_positions.tsv that seem to show the problem and also tested on one of the files of the @mukamel-lab that should contain NaNs and zeros. The output of bigwig-loader and PyBigWig are identical.

@jorenretel
Copy link
Collaborator Author

Okay, I think I can replicate now. It happens when values are returned in windows. Working on a fix.

@jorenretel jorenretel force-pushed the debug/default_value_error branch from 7d423cb to a544afc Compare April 24, 2025 20:36
@jorenretel
Copy link
Collaborator Author

@mukamel-lab, I think this last commit should solves the problem. If it indeed does I will merge this and create a new release to pypi and the conda channel.

@mukamel-lab
Copy link

@mukamel-lab, I think this last commit should solves the problem. If it indeed does I will merge this and create a new release to pypi and the conda channel.

I've tried out the new branch debug/default_value_error but it still doesn't seem to work properly when I use window_size. Maybe I'm not invoking it properly? Thanks again for your help with this.

…tion outputs the same as the cp.nanmean on the full matrix
@jorenretel
Copy link
Collaborator Author

@mukamel-lab, when you create

@mukamel-lab, I think this last commit should solves the problem. If it indeed does I will merge this and create a new release to pypi and the conda channel.

I've tried out the new branch debug/default_value_error but it still doesn't seem to work properly when I use window_size. Maybe I'm not invoking it properly? Thanks again for your help with this.

I added tests that take bigwig files and takes a batch from them with window_size=1 and window_size=128. Then I compare whether Batch(window_size=128) is the same as the Batch(window_size=1) with a nanmean over the window. Are you creating the BigWigDataset (or PytorchBigWigDataset) something like this?

from bigwig_loader.dataset import BigWigDataset

import cupy as cp

dataset_with_window = BigWigDataset(
    regions_of_interest=merged_intervals,
    collection=bigwig_path,
    reference_genome_path=reference_genome_path,
    sequence_length=2048,
    center_bin_to_predict=2048,
    window_size=128,
    batch_size=32,
    batches_per_epoch=10,
    maximum_unknown_bases_fraction=0.1,
    default_value=cp.nan,
    return_batch_objects=True
)

@jorenretel
Copy link
Collaborator Author

@mukamel-lab, I think this last commit should solves the problem. If it indeed does I will merge this and create a new release to pypi and the conda channel.

I've tried out the new branch debug/default_value_error but it still doesn't seem to work properly when I use window_size. Maybe I'm not invoking it properly? Thanks again for your help with this.

Sorry to ask, but did you git pull the latest changes on this branch?

@mukamel-lab
Copy link

mukamel-lab commented Apr 25, 2025

@mukamel-lab, I think this last commit should solves the problem. If it indeed does I will merge this and create a new release to pypi and the conda channel.

I've tried out the new branch debug/default_value_error but it still doesn't seem to work properly when I use window_size. Maybe I'm not invoking it properly? Thanks again for your help with this.

Sorry to ask, but did you git pull the latest changes on this branch?

I found that your test (test_get_values_from_intervals_edge_case_1) fails if I use a default value other than 0 or nan. I realized this can be fixed by making sure default_value is np.float32.

However, I'm still having trouble with PyTorchBigWigDataset filling in 0 instead of default_value for bins that are not at the right-hand end of the query region.

My code looks like this:

 dataset = PytorchBigWigDataset(
          regions_of_interest=regions,
          collection=metadata['path'],
          reference_genome_path=reference_genome_file,
          sequence_length=binsize*nbins,
          center_bin_to_predict=binsize*nbins,
          window_size=binsize,
          maximum_unknown_bases_fraction=0.1,
          sequence_encoder="onehot",
          return_batch_objects=True,
          default_value=cp.nan,
        repeat_same_positions=False, # Does this lead to reproducible samples??
      )

dataloader=DataLoader(dataset, num_workers=0, batch_size=None)
batch=next(iter(dataloader))

@jorenretel
Copy link
Collaborator Author

This is very helpful. My excuses. I am drilling into this now.

…ared to the output of pyBigWig with a window function applied afterwards. Parametrized with different default values.
@mukamel-lab
Copy link

This is very helpful. My excuses. I am drilling into this now.

I came up with a fix (using chatGPT! -- I have no experience with cuda programming) which appears to work for me. Please check if this looks okay, and thanks!

https://github.com/mukamel-lab/bigwig-loader/tree/mukamel-lab-patch-1

…stom_position_sampler and custom_track_sampler options to the dataset objects.
@jorenretel
Copy link
Collaborator Author

jorenretel commented Apr 29, 2025

@mukamel-lab will look into your cuda code. In the meanwhile I added more tests. There is also a test using the actual PytorchBigWigDataset (instead of the stuff underneath) and I still have problems replicating what you're seeing (also on the bigwig file you send me).

To make things easier to replicate I added a "custom_position_sampler" to the dataset objects (which can just be an Iterable of tuples of chromosome, center position like [("chr1", 73674382), ("chr6", 725209)....]. Could you maybe change this test, so it fails (on your file):

def test_pytorch_dataset_with_window_function(
?
Or tell me how what that test tests is not what you mean.

I wondered, when you figured out that the "default_value" was incorrectly not a float32 when not 0 or NaN, did you also try cp.float32(cp.nan) as default_value? For which, thanks again. Because what could have happened was that positions in the float32 tensor were being overwritten with 64 bits somehow in a row, leaving the first 32 bits of the 64 bit NaN in each position?? I am just guessing at this point what could be happening. But it's so hard since (unlike before) I am not finding those zeros.

@mukamel-lab
Copy link

@mukamel-lab will look into your cuda code. In the meanwhile I added more tests. There is also a test using the actual PytorchBigWigDataset (instead of the stuff underneath) and I still have problems replicating what you're seeing (also on the bigwig file you send me).

To make things easier to replicate I added a "custom_position_sampler" to the dataset objects (which can just be an Iterable of tuples of chromosome, center position like [("chr1", 73674382), ("chr6", 725209)....]. Could you maybe change this test, so it fails (on your file):

def test_pytorch_dataset_with_window_function(

?
Or tell me how what that test tests is not what you mean.
I wondered, when you figured out that the "default_value" was incorrectly not a float32 when not 0 or NaN, did you also try cp.float32(cp.nan) as default_value? For which, thanks again. Because what could have happened was that positions in the float32 tensor were being overwritten with 64 bits somehow in a row, leaving the first 32 bits of the 64 bit NaN in each position?? I am just guessing at this point what could be happening. But it's so hard since (unlike before) I am not finding those zeros.

It works for me now! I think I must have been incorrectly loading the wrong version of the module. I am now able to get the expected behavior from PytorchBigWigDataset. Thank you!!

@jorenretel
Copy link
Collaborator Author

Great! I was already afraid we were chasing a Heisenbug.

@jorenretel jorenretel merged commit 2185b35 into main Apr 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants