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

README: A more engaging code snippet example #635

Merged
merged 2 commits into from
Jul 23, 2023

Conversation

omesser
Copy link
Contributor

@omesser omesser commented Jul 23, 2023

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

As discussed in an issues in studio#6422) (private repo) - suggested some changes to this snippet:
copy-pasting motivation:

Yes - a working snippet is what I was driving it.
Looking at the snippet in the readme I suggest to consider updating maybe - that's why I looked at some competitors and compiled a new one before.

first order problem IMO is having a snippet that works out of the box
2nd order problem - polish how this snippet looks - so it's short and clean and nice, but also interesting.
things that bother me today a bit about the snippet from the readme(⚠️ : opinionated / style!):

  • direct access to sys.argv (ugly)
  • I can't run it "cleanly" -python train.py - fails in an ugly way (πŸ’­ mental mode - I'm just copy-pasting in "autopilot mental mode" to test studio - I don't want to have to make a decision about epoch parameter)
  • 4 metrics/plots - train/eval is redundant (the same, also "eval called "val" ? πŸ˜– )
  • graphs aren't "interesting" to my taste (linear + noise):
    screenshot Screenshot 2023-07-09 at 23 47 40
    that's a personal "peeve" - it's not bad per-se, just that I saw an easy way to improve (from wandb, see below).
  • exp finishes too quickly - had to run several times to "catch" the "liveness" of the metrics

Graphs for new snippet

screenshot 252168487-e814ec47-8123-4f35-962a-b365f6c716f2

wdyt about this instead of the existing one @daavoo?

@omesser omesser requested a review from daavoo July 23, 2023 09:25
from dvclive import Live

params = {"learning_rate": 0.002, "optimizer": "Adam", "epochs": 20}
Copy link
Contributor

@daavoo daavoo Jul 23, 2023

Choose a reason for hiding this comment

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

I don't love that params don't have an effect nor change across runs, but not a blocker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is a for a fake feel of familiarity / realism. But I've seen this in other places and I think since this is very very explicitly "fake" it's not misleading anyone (personal feel)

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (d39bd1c) 89.53% compared to head (6712a54) 89.53%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #635   +/-   ##
=======================================
  Coverage   89.53%   89.53%           
=======================================
  Files          43       43           
  Lines        2992     2992           
  Branches      250      250           
=======================================
  Hits         2679     2679           
  Misses        274      274           
  Partials       39       39           

β˜” View full report in Codecov by Sentry.
πŸ“’ Do you have feedback about the report comment? Let us know in this issue.

@daavoo daavoo merged commit 50f15a7 into iterative:main Jul 23, 2023
11 checks passed
@omesser omesser changed the title README: A more engaging code snippet ecample README: A more engaging code snippet example Jul 23, 2023
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.

None yet

3 participants