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

Adding a reset_gpu method to the BigWigCollection and Dataset objects… #3

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

jorenretel
Copy link
Collaborator

…. This can be handy when an existing dataset or collection object should be brought to a new gpu device.

…. This can be handy when an existing dataset or collection object should be brought to a new gpu device.
@jorenretel jorenretel requested a review from ap-- as a code owner January 16, 2024 12:38
Comment on lines 87 to 95
def _get_decoder(self) -> Decoder:
if self._decoder:
return self._decoder
self._decoder = Decoder(
max_rows_per_chunk=self.max_rows_per_chunk,
max_uncompressed_chunk_size=self.max_rows_per_chunk * 12 + 24,
chromosome_offsets=self.local_chrom_ids_to_offset_matrix,
)
return self._decoder
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use a cached_property here to simplify this a bit.

You'd just need to switch your code in reset_gpu to delete from __dict__

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, I am mostly using this pattern because mypy tends to complain when I set (and type) self._decoder in init and then have a property:

@property
def decoder(self):
    ...

I can be wrong. Does cached_property fix exactly that problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep.

You get rid of self._decoder and you just have:

@cached_property
def decoder(self) -> Decoder:
    return Decoder(...)

Clearing the cache looks like this:

def clear(self):
    if "decoder" in self.__dict__:
        del self.__dict__["decoder"]

It's useful to do it like that if all you want is late initialization of self.decoder

@@ -75,6 +73,27 @@ def run_indexing(self) -> None:
def __len__(self) -> int:
return len(self.bigwigs)

def reset_gpu(self) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this ever called on the collection level?

It seems you have to repeat the method to have to delegate from the top levels down.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Because a BigWigDataset object can take a BigWigCollection object as one of the init args. This can be handy when you want to create train, val and test datasets, all based on the same collection of bigwig files, but with different "regions of interest" from which the samples are taken:

https://github.com/pfizer-opensource/bigwig-loader/blob/d84a233a6214162c3e7178684f43d8a198fb88d0/bigwig_loader/dataset.py#L61C1-L62C1

@jorenretel jorenretel self-assigned this Jan 26, 2024
@jorenretel jorenretel marked this pull request as draft January 26, 2024 13:44
@jorenretel jorenretel marked this pull request as ready for review January 26, 2024 14:32
@jorenretel jorenretel merged commit 2a1dd48 into main Jan 26, 2024
1 check passed
@jorenretel jorenretel deleted the feature/reset_gpu branch January 26, 2024 14:43
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.

2 participants