-
Notifications
You must be signed in to change notification settings - Fork 350
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
base: main
Are you sure you want to change the base?
Conversation
Hmm, not sure how to test datamodule plotting now that this is gone. Not even sure if we need datamodule plotting anymore. |
Some datamodules also have custom plot methods. |
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 |
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 |
There was a problem hiding this 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
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. |
Per the comment in #2138 remove figure logging