microsoft
Remove logging of figures
Per the comment in https://github.com/microsoft/torchgeo/issues/2138 remove figure logging
Hmm, not sure how to test datamodule plotting now that this is gone. Not even sure if we need datamodule plotting anymore.
Did we decide we want to remove datamodule plotting, or just find a different way to test it? Not sure if @isaaccorley's callback suggestion still requires datamodule plotting methods.
I would be in favor of just keeping the datamodule.plot as a reference to the dataset so users don't have to dig to find out how to access the dataset to plot. But not actually testing it if it's just a reference to the dataset plot method.
Then it would be clear that a user can do trainer.datamodule.plot without having to do trainer.datamodule.<split>_dataset.plot
Well we have to test it if we want to maintain 100% coverage. Can you share an example callback you use?
Ping @isaaccorley, we need a callback to get test coverage if you want to keep datamodule.plot() methods in place.
We can't merge without full coverage. We can't get full coverage without testing the plotting or removing the plotting.
@adamjstewart is testing or removing plotting in scope of this PR? Seems a shame to remove it, but also not sure how you would even test plotting
I can't merge without 100% test coverage, which makes it within the scope of this PR. Testing would involve adding a callback to our trainer tests to do what our trainers used to do manually.
Hmm the copilot screwed up this branch, easier to create a new PR when I have time to work on it properly
FWIW, I played around with this and couldn't find an obvious way to write a callback to plot images. We may want to reach out to the Lightning folks to see if there are any suggestions for logger-agnostic plotting.
In my own approach I've switched to logging artefacts in the test step and using a global counter for the number - I've found this gives me better control. Last time I asked Lightning there was no plans to standardise the logging of artefacts, so the best we could do is support (1) the most common logger, and have users override it (2) support a number of loggers, somehow