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

Use ProgressMeter to print current loss #1146

Closed
wants to merge 10 commits into from

Conversation

AStupidBear
Copy link
Contributor

@AStupidBear AStupidBear commented Apr 25, 2020

image

@bhvieira
Copy link
Contributor

That would remove the dependency on Juno, which I'm led to believe was only added for @progress. Is that so @MikeInnes ?

@AStupidBear
Copy link
Contributor Author

@bhvieira

@render Juno.Inline t::Tree begin

Juno is still used there to print trees

@MikeInnes
Copy link
Member

I think something like this would be useful, although I don't think we can accept a PR that tries to do a number of different things at once.

I would prefer not to lose the Juno integration; does Juno work with ProgressMeters at this point?

@johnnychen94
Copy link
Contributor

johnnychen94 commented Apr 28, 2020

does Juno work with ProgressMeters at this point?

Although the behavior is slightly different, TerminalLoggers.jl might be the right tool; it uses the same API setting as Juno: ProgressLogging

@AStupidBear
Copy link
Contributor Author

@johnnychen94 @MikeInnes I added progressive loss printing for DiffEqFlux.sciml_train before, but that's not as pretty as this PR with ProgressMeter.jl. See JuliaLogging/ProgressLogging.jl#23

@AStupidBear
Copy link
Contributor Author

I think something like this would be useful, although I don't think we can accept a PR that tries to do a number of different things at once.

@MikeInnes Now only the printing part is reserved. Can I say this PR now only do one thing?

@CarloLucibello
Copy link
Member

This PR is also adding an extra for loop over the epochs. I think this is convenient to have, but certainly not related to the progress bar infrastructure

@AStupidBear
Copy link
Contributor Author

This PR is also adding an extra for loop over the epochs. I think this is convenient to have, but certainly not related to the progress bar infrastructure

Now the loop over epochs is removed. Can this PR be merged?

@CarloLucibello
Copy link
Member

I like ProgressMeter, but we would be losing Juno support. Is there a way we can have both?

@AStupidBear
Copy link
Contributor Author

I like ProgressMeter, but we would be losing Juno support. Is there a way we can have both?

Added back!

@MikeInnes
Copy link
Member

To be honest, this is a fair bit of extra complexity when our train! function is meant to be very simple (not least so people can just copy it and do their own thing). It also seems quite specific; I'm sure there are people for whom the particular behaviour of verbose is useful, but anyone who wants something a bit different is back to writing the for loop again.

I suspect it would be better to experiment with more powerful training loops in a separate package, rather than adding feature after feature to Flux proper (bearing in mind we are constrained by stability). That would give you a lot of flexibility to experiment, and as the design evolves we can consider whether it's time to make it official.

Of course, I think if we could simply make @progress support the terminal as well as Juno without code changes here, that'd be a no brainer.

@AStupidBear
Copy link
Contributor Author

@AStupidBear AStupidBear closed this Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants