Skip to content

Conversation

@rijobro
Copy link
Contributor

@rijobro rijobro commented Jan 22, 2021

Will enable (dictionary example given, array is the same):

transforms = Compose([
    LoadImaged(keys=["image","label"]),
    ToTensord(keys=["image","label"]),
    CopyToDeviced(keys=["image","label"], device=device),
])

for batch_data in train_loader:
    im, label = batch_data["image"], batch_data["label"]

removing the need for:

for batch_data in train_loader:
    im, label = batch_data["image"].to(device), batch_data["label"].to(device)

Status

Ready

Types of changes

  • Non-breaking change (fix or new feature that would not break existing functionality).
  • New tests added to cover the changes.
  • Integration tests passed locally by running ./runtests.sh --codeformat --coverage.
  • Quick tests passed locally by running ./runtests.sh --quick.
  • In-line docstrings updated.
  • Documentation updated, tested make html command in the docs/ folder.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro rijobro added the enhancement New feature or request label Jan 22, 2021
@Nic-Ma
Copy link
Contributor

Nic-Ma commented Jan 22, 2021

Hi @rijobro ,

Thanks for your good idea.
I have a question In my simple mind: if moving data to GPU in a transform in every process, it will batch together in DataLoader later and the batch data is not contiguous in GPU memory, maybe the performance is worse than "moving to GPU" after batching? Just curious, maybe you can provide some test results to compare later.

Thanks.

@rijobro
Copy link
Contributor Author

rijobro commented Jan 22, 2021

Thanks @Nic-Ma I hadn't thought of that. I'll have a look!

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
This reverts commit e56e386.

Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
Signed-off-by: Richard Brown <33289025+rijobro@users.noreply.github.com>
@rijobro
Copy link
Contributor Author

rijobro commented Jan 25, 2021

Hi @Nic-Ma was looking into this, but it seems it's a bad idea, since each of the subprocesses from the DataLoader would have to share GPU memory across processes. See here and here.

Hence, I'll close this PR.

@rijobro rijobro closed this Jan 25, 2021
@wyli
Copy link
Contributor

wyli commented Jan 25, 2021

but I think copy_to_device is still a useful utility, isn't it?

@rijobro
Copy link
Contributor Author

rijobro commented Jan 25, 2021

that functionality still exists in the lr_finder PR, I could split it into its own PR if you'd like. But I think if we're about to merge the lr_finder PR, there's not much point.

@rijobro rijobro deleted the CopyToDevice_transform branch May 4, 2021 10:38
@rijobro rijobro mentioned this pull request Aug 17, 2021
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants