Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove logging of figures #2184

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

robmarkcole
Copy link
Contributor

Per the comment in #2138 remove figure logging

@github-actions github-actions bot added the trainers PyTorch Lightning trainers label Jul 22, 2024
@adamjstewart
Copy link
Collaborator

Hmm, not sure how to test datamodule plotting now that this is gone. Not even sure if we need datamodule plotting anymore.

@adamjstewart adamjstewart added this to the 0.6.0 milestone Jul 22, 2024
@github-actions github-actions bot added the datamodules PyTorch Lightning datamodules label Jul 22, 2024
@github-actions github-actions bot added the testing Continuous integration testing label Jul 22, 2024
@adamjstewart
Copy link
Collaborator

Some datamodules also have custom plot methods.

@adamjstewart adamjstewart added the backwards-incompatible Changes that are not backwards compatible label Jul 23, 2024
@adamjstewart
Copy link
Collaborator

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.

@isaaccorley
Copy link
Collaborator

isaaccorley commented Jul 24, 2024

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

@adamjstewart
Copy link
Collaborator

Well we have to test it if we want to maintain 100% coverage. Can you share an example callback you use?

@adamjstewart
Copy link
Collaborator

Ping @isaaccorley, we need a callback to get test coverage if you want to keep datamodule.plot() methods in place.

Copy link
Collaborator

@isaaccorley isaaccorley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100% approve of this

@adamjstewart
Copy link
Collaborator

We can't merge without full coverage. We can't get full coverage without testing the plotting or removing the plotting.

@adamjstewart adamjstewart modified the milestones: 0.6.0, 0.7.0 Aug 27, 2024
@robmarkcole
Copy link
Contributor Author

@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

@adamjstewart
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompatible Changes that are not backwards compatible datamodules PyTorch Lightning datamodules testing Continuous integration testing trainers PyTorch Lightning trainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants