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

added get-started-experiments generation code #44

Merged
merged 42 commits into from
Jul 14, 2021

Conversation

iesahin
Copy link
Contributor

@iesahin iesahin commented May 19, 2021

This PR contains the source code and generation script for https://github.com/iterative/get-started-experiments

It includes the general generate.bash script that prepares the build directory, calls the generate-experiments.bash and writes a push script that pushes the tags to the repository.

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

Initial review (mostly just *.md) for now.

  • Since you use vim, can you use a plugin which strips trailing spaces (line ending) as well as end-of-file (single-newline)?
  • Also don't use [links][like this] unless you re-use them. Otherwise it's just a liability (broken/missing links) - spotted at least one I think.

get-started/code-experiments/.gitignore Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
get-started/code-experiments/README.md Outdated Show resolved Hide resolved
@iesahin iesahin self-assigned this May 19, 2021
@jorgeorpinel
Copy link
Contributor

May want @dberenbaum's review instead of mine in this one 🙂

Just one minor Q: is "generate.bash" Bash-specific or is it a general Shell script (.sh extension) ?

@iesahin
Copy link
Contributor Author

iesahin commented May 20, 2021

* Since you use `vim`, can you use a plugin which strips trailing spaces (line ending) as well as end-of-file (single-newline)?

Actually I used VS Code with Remote editing in this PR. Vim has these config, I'll take care of this.

@iesahin
Copy link
Contributor Author

iesahin commented May 20, 2021

* Also don't use `[links][like this]` unless you re-use them. Otherwise it's just a liability (broken/missing links) - spotted at least one I think.

I'm using these because of readability. I read these documents usually as text. They fit into the paragraph and don't distract the flow. Broken links are a different matter, though, I don't put the links while writing and forget to put them sometimes. They are easy to spot though.

@casperdcl
Copy link
Contributor

It looks like this PR has already been deployed to https://github.com/iterative/get-started-experiments before being reviewed & merged 🤔

Anyway for all these generation-type stuff we should also have a note at the bottom of generated READMEs linking back to this source repo.

@iesahin
Copy link
Contributor Author

iesahin commented May 25, 2021 via email

@casperdcl
Copy link
Contributor

I generated and pushed the repos because I think it's easier to see the end product in review.

best to push to a temp repo rather than an in-production repo (otherwise defeats at least one of the objectives of review)

I see the generation code as an internal stuff while the resulting repos as the product, but I may add the links to this repo if it feels better.

exactly. Advanced users (and us too, for our own sanity) care about internal stuff being linked (a one-line contributing section stating "please open PRs in the source repo if you find a bug," etc.)

@shcheklein
Copy link
Member

Nice @iesahin ! It's way simpler now.

params.py now contains a flat list of 4 parameters: instead of hierarchical model.cnn.conv_units, we now have conv_units.

I would keep one nested level (to show that it's possible)

Also a few other comments:

  • can we use images and labels instead of .gz?
  • simplify the code also to some reasonable extent

@shcheklein
Copy link
Member

But overall, I would try at this point to write some documentation first and see what can we get there - then do other iterations on this.

@iesahin
Copy link
Contributor Author

iesahin commented Jul 10, 2021

I think this is done. I'm about to merge if no more changes requested. @shcheklein @jorgeorpinel @casperdcl

README.md Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved

### Parameters

There are two parameters in the project. They are set in `params.yaml`. `models.conv_units` defines the number of convolutional units in the model, and `train.epochs` sets the number of epochs to train the model.
Copy link
Member

Choose a reason for hiding this comment

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

preferable use the same style across all *.md

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

some minor comments here and there, looks good otherwise to me.

@iesahin
Copy link
Contributor Author

iesahin commented Jul 14, 2021

@shcheklein About the root .gitignore file I probably deleted the file mistakenly because I never had any intention to work on files outside of the get-started-experiments directory. I've just updated the README recently. I've reinstated the global .gitignore.

@iesahin iesahin merged commit d1552b3 into iterative:master Jul 14, 2021
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

6 participants