3 double drop

[philswatton]

These commits do 2 broad things to contribute to #3:

• Turn the CIFAR10DataModuleDrop class's dataloader into one that can mange dropping the same % from both A and B, while still being useful for the case where we only want to drop from one but not the other
• Adds a test to ensure that when dropping from both, we get complete non-overlap as expected

[lannelin]

I think it'd be good to whiteboard around design here so I'll finish up my review for now and we can discuss in person!

[philswatton]

I've overhauled the code in line with our discussions. The DMPair class now takes drop percentages as input, sets up a CIFAR data module from pytorch, then uses this to generate two instances of the DMCifarSubset class. This should in theory improve the overall efficiency of the code.

What isn't handled in the current code is:

• Generalisation to allow inclusion of other datasets
  • This shouldn't be too difficult to do, would mainly need arguments for datasets A and B, additional subset classes for other datamodules (e.g. MINST), and different behaviours if we want to drop from both A and B (in the case where A and B are not the same, we don't need to consider overlap)
  • However, this is better handled in the more distant future
• Handling of transformations, and making them different across A and B
  • This is part of CIFAR-10 dataset in pipeline #3, however I think this is best handled in a separate PR
• Validation splits
  • In line with today's discussion, this is something we'll need to consider, especially where A and B are from the same source dataset. However, I think that's a bridge better crossed in the future.

[jack89roberts]

Nice, I've added a few comments (some of which may just be me misunderstanding what's going on).

[lannelin]

Structure looks good.

A couple of minor comments.

Further, could you see if you can iterate through the resulting dataloaders and whether they behave as expected? I get an error trying to loop through dmpair.A.train_dataloader()
would be good to test looping through and count the no. of items. This should probably be a separate test as otherwise just testing the inds which are separate.

[philswatton]

I've added several updates to the PR:

• Taken the double test-train split out of the DMPair class into its own function
• Tests now cover A and B
• Extra test covering batching over the dataloader, for a total of 8 tests up from 3
• Variable names hopefully clearer
• Extra comments, var names for equations hopefully also clarify stuff

I haven't addressed the .setup() comment yet. Mainly becuase looking at the source code (https://github.com/Lightning-AI/lightning-bolts/blob/c26c8d8f407de386038d5fb13297233a8aa052e7/pl_bolts/datamodules/vision_datamodule.py#L10) this is the stage at which transformations etc are applied, so I'd rather handle it an another PR aimed at dealing with the transforms

[jack89roberts]

Not gone through everything in full but LGTM - I just have some minor comments on the formatting/structure of the new splitting function.

[lannelin]

as discussed on slack, let's add the setup override even if it's just a copy+paste of the transform code in the parent. In addition, could you add some comments and logging to clarify that we will not be doing late loading and that the data setup is happening in the initialisation of the class?
I suggest just using logging with a logger set up at module level logger = logging.getLogger(__name__) (as per https://docs.python.org/3/howto/logging.html#advanced-logging-tutorial) and a warning level message in __init__ like logger.warning('sensible message here')

[lannelin]

Looks good, nice work. Very minor comment re. moving the logging statement. Happy for it to be merged after.

[jack89roberts]

🚀