tianshou icon indicating copy to clipboard operation
tianshou copied to clipboard

Batch: remove `is_empty`

Open MischaPanch opened this issue 10 months ago • 24 comments

It's not really a useful method and bloats the interface. The documentation already shows the intricacies of using it with recurse=True or recurse=False.

It's more explicit to replace it with

  1. batch.is_empty() -> len(batch.get_keys()) == 0
  2. batch.is_empty(recurse=True) -> len(batch) == 0

Solving this means using the IDE's find usages, applying the above replacements, and removing is_emtpy from Batch and BatchProtocol

MischaPanch avatar Apr 12 '24 10:04 MischaPanch

@DarkTechPirate wanna have a look at this one? It's a fairly small thing and a good way to get started with contributing :)

MischaPanch avatar Apr 12 '24 10:04 MischaPanch

@MischaPanch yea sure

DarkTechPirate avatar Apr 12 '24 10:04 DarkTechPirate

Cool, thanks!

MischaPanch avatar Apr 12 '24 11:04 MischaPanch

image

can this also be changed into len(batch.get_keys()) == 0

DarkTechPirate avatar Apr 12 '24 12:04 DarkTechPirate

Not sure what you mean. How is this related to is_empty()?

MischaPanch avatar Apr 12 '24 12:04 MischaPanch

in 5th line we are using is_empty() to check it right!? if we remove that fun it wont work , so we need to replace it with something right

DarkTechPirate avatar Apr 12 '24 14:04 DarkTechPirate

Yes, you're right, hadn't seen it. I think a full-text search of is_empty should reveal all usages, in case the find usages of your IDE misses some

MischaPanch avatar Apr 12 '24 23:04 MischaPanch

@MischaPanch I'd be glad to assist with this one for review in PR.

dantp-ai avatar Apr 15 '24 16:04 dantp-ai

@MischaPanch I'd be glad to assist with this one for review in PR.

Sounds good, thanks!

MischaPanch avatar Apr 15 '24 16:04 MischaPanch

@DarkTechPirate do you have an ETA for when you could submit a PR?

MischaPanch avatar Apr 17 '24 11:04 MischaPanch

@MischaPanch so can we have a goggle meet or something as soon all my doubts clear i can finish it in 1 or 2 hours , let me know is it even possible ?

DarkTechPirate avatar Apr 17 '24 19:04 DarkTechPirate

@dantp-ai hey sure , we shall also have a meeting when you are free , lmk

DarkTechPirate avatar Apr 17 '24 19:04 DarkTechPirate

I won't have time until the end of next week, but @dantp-ai knows what this issue is about, so if the two of you have time to talk to each other, that would be the fastest option :). It's not a big change

MischaPanch avatar Apr 17 '24 19:04 MischaPanch

@DarkTechPirate I'd be happy to help. We can look at it tomorrow.

dantp-ai avatar Apr 17 '24 20:04 dantp-ai

@MischaPanch Okay sure , we will finish it off asap

DarkTechPirate avatar Apr 17 '24 20:04 DarkTechPirate

Thank you two, highly appreciate it!

MischaPanch avatar Apr 17 '24 20:04 MischaPanch

@dantp-ai how to contact you , discord or you people use something else !? its my first time working with someone , so sorry for inconvenience!

DarkTechPirate avatar Apr 17 '24 20:04 DarkTechPirate

@DarkTechPirate Once you have a solution (a work-in-progress is fine, though a complete solution is greatly appreciated), please open a PR and we can review the code together here on GitHub.

The above description of the issue is instructive and should contain all the hints. If you have any doubts, feel free to write them down here and I will be able to help you.

I assume you have already read the document on contributing to the codebase?

dantp-ai avatar Apr 18 '24 09:04 dantp-ai

@DarkTechPirate How can I help?

dantp-ai avatar Apr 23 '24 14:04 dantp-ai

@dantp-ai I have made some changes , but idk if it is correct or wrong , so let me send a pull request shortly and kindly can you go through it ?

DarkTechPirate avatar Apr 23 '24 14:04 DarkTechPirate

Yes, let's do like that. Looking forward to the PR.

dantp-ai avatar Apr 23 '24 14:04 dantp-ai

@DarkTechPirate I didn't see the PR. If you made some changes let's review them together in the PR even if you are unsure of the changes. Pls add me as reviewer. Thank you!

dantp-ai avatar Apr 25 '24 14:04 dantp-ai

okay

DarkTechPirate avatar Apr 25 '24 16:04 DarkTechPirate

@dantp-ai check #1125 Seems like i kinda messed up , T_T

DarkTechPirate avatar Apr 25 '24 16:04 DarkTechPirate