can you explain why this is correct if self.precompute?

The index idx is not used at all, so this looks wrong.

Its being used under __item_tensor__ which is just a rename of the original __getitem__ its input param is not changed including it accepting the idx.

But the mechanism at which it gets idx is different.

When self.precompute=True, __item_tensor__ gets the idx first, so when this happens, the second time we receive idx in __getitem__ it will now be a different set. This will change the actual behavior of the original __getitem__ so we dont use it, instead we follow the order from self.precompute_cache which contains the correct/original order of idx from the first call.

When self.precompute=True, the order at which the idx gets obtained by __item_tensor__ is as follows:

1. create Dataloader by calling to_dataloader
2. this calls __precompute__
3. __precompute__ follows TimeSynchronizedBatchSampler to get idx
4. passes idx first sequences to __item_tensor__

Caveat:
So when Dataloader generates its own idx from its own Sampler it is not used, if we use the Dataloader idx we will NOT get the benefit of this PR, as that idx is being generated real time i guess.

So, instead of retrieving from cache, the effect is we will be back to computing original __getitem__ on each call, which is the slow path all over again, starving the GPU, technically is the source of slowness.

However, if a user wishes to use the idx of the Dataloader, the default will just skip precomputation, and get the original slow path from __getitem__ -> __item_tensor__ - which is the same vanilla __getitem__ anyway.

When self.precompute=True, __item_tensor__ gets the idx first, so when this happens, the second time we receive idx in __getitem__ it will now be a different set.

Sorry, I still do not get it. Can you explain which dunders are called from external, and which are only used internally? I thought that __getitem__ gets called - so when it does not use idx, the information is not passed on.

TLDR
Does this PR use idx from Dataloader? YES and NO. If using defaults (precompute=False), then YES. But if using accelerated path (precompute=True), then NO.

EXPLANATIONS

• Vanilla

  Dataloader -> pass idx -> __getitem__() -> use idx

• PR - Revised vanilla - slow path (precompute=False)

  Dataloader -> pass idx -> __getitem__() -> pass idx -> 
  __itemtensor__() -> use idx

  Notes:

  • __getitem__() is revised so it can switch to vanilla and accelerated based on precompute value, in this case, its using vanilla.
  • the __itemtensor__() is just the renamed __getitem__() so its the usual calculation path when precompute=False

  Verdict:

  • idx from Dataloader is used, as usual.

• PR - Accelerated path (precompute=True)

  Two steps happen here

  • a. Pre computation routine

    to_dataloader () -> __precompute__() -> generates idx from TSDS -> 
    loop on every idx -> pass each idx -> __itemtensor__() -> use idx -> 
    save to data to cache -> move to the next idx -> repeat

    This is essentially how Dataloader uses __getitem__(). It calls __getitem__() and pass idx to it, repeats until all idx exhaust the data. But, what I did in this PR is used the FOR LOOP to retrieve idx in advance and exhaust the data so we can pre-calculate it without calling the Dataloader.

  • b. Dataloader call

    Since we have cached the result of __itemtensor__() in advanced. Meaning __itemtensor__() has the pre-computed the data on each specific idx. We just retrieve it.

    Dataloader -> pass idx -> __getitem__() -> idx ignored ->  use cache sequence

    Note:

    • notice, we dont need to call the __itemtensor__() this time since it already did its job

    Verdict:

    • idx from Dataloader is not essential anymore, since another idx source has been used, specifically from TimeSynchronizedBatchSampler

@jobs-git, apologies, I still do not get it.

Since we have cached the result of itemtensor in advanced. meaning itemtensor has the precomputed the data on each specific idx. We just retrieve it.

As far as I understand: you are not retrieving the expected result at idx. You are retrieving the results in sequence, irrespective of which idx gets queried. Therefore, this is not the correct logic for __getitem__.

Please let me know if you disagree.

@phoeenniixx, @PranavBhatP, @agobbifbk, @fnhirwa - can you perhaps also have a look? I also do not fully understand the explanation.

For us to be able to accelerate this (precomputation=True), we need to query the idx's in advance (not wait for Dataloader to give it one by one) to perform precomputation. This is what the PR does.

I am confused by your meaning of sequence, since idx's from TimeSynchronizedBatchSampler is always random when shuffle=True, we just saved the result sequentially so retrieval is also sequence.

As mentioned previously, acceleration requires obtaining the idx's in advance, otherwise, we can't perform pre-computation.

@fkiraly I think what you prefer implemented is obtaining the precomputed values from real-time idx? This is what I prefer too, until I saw that the algorithm seems to be normalizing on a given idx.

That is when I realized we cant accelerate this if we wait for Dataloader, so we need to obtain idx's in advance from a different mechanism. Thus this PR.

May I ask what is the actual concern if the accelerated path uses a different mechanism to obtain random idx?

Of course, the exact training trajectory would likely be different, but a properly trained model should resolve to the same/similar outcome. If I could present prediction results, would that settle the concern for accelerated path using a different random idx generator? Just let me know.