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

new readme #690

Merged
merged 10 commits into from
Mar 3, 2023
Merged

new readme #690

merged 10 commits into from
Mar 3, 2023

Conversation

hstojic
Copy link
Collaborator

@hstojic hstojic commented Feb 3, 2023

this will likely trigger changes to contribution guidelines
we also need to setup codecov.io account

Copy link
Collaborator

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

Big improvement! A few initial comments.

README.md Outdated
A Bayesian optimization toolbox built on [TensorFlow](https://www.tensorflow.org/). Trieste supports Python 3.7 onwards and uses [semantic versioning](https://semver.org/).
[![PyPI](https://img.shields.io/pypi/v/trieste.svg)](https://pypi.org/project/trieste)
[![License](https://img.shields.io/badge/license-Apache-green.svg)](LICENSE)
[![Quality checks](https://github.com/secondmind-labs/trieste/actions/workflows/quality-checks.yaml/badge.svg)](https://github.com/secondmind-labs/trieste/actions?query=workflows%3Aquality-checks)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the badge we want here? AFAICT it will show red whenever any unmerged PR is failing some tests. Maybe we want https://github.com/secondmind-labs/trieste/actions/workflows/develop-checks.yaml/badge.svg instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, wasn't sure which one exactly to show... it also shows names of our workflows automatically, can we make those names more human readable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, found a way to show the names nicely, we need to add a name field to our workflows - I'll do that

I agree that quality-checks doesn't make sense as it run on every PR, but develop-checks though runs only the slow tests so that doesn't seem appropriate as well

actually, what we need is checks on master branch, running everything we have, thats what matters here, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

develop is essentially master in trieste, so develop-checks is the closest we have (though I could extend it to also rerun the quick tests, even though these need to have already passed for you to merge)

Copy link
Collaborator Author

@hstojic hstojic Feb 24, 2023

Choose a reason for hiding this comment

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

we often had issues with develop checks not passing, it seems bad that anytime that some PR is merged we could get failing signs here, while the official release is healthy - I think we simply need another GH action that runs everything when there is a release, i.e. something really merged to master

README.md Outdated Show resolved Hide resolved
### Installation
[Documentation (release)](https://secondmind-labs.github.io/trieste/1.0.0/index.html) |
[Documentation (develop)](https://secondmind-labs.github.io/trieste/develop/index.html) |
[Tutorials](https://secondmind-labs.github.io/trieste/1.0.0/tutorials.html) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Though I guess it won't help for these links. Unless you'd also like me to redirects at https://secondmind-labs.github.io/trieste/tutorials.html and https://secondmind-labs.github.io/trieste/autoapi.html?

Copy link
Collaborator Author

@hstojic hstojic Feb 3, 2023

Choose a reason for hiding this comment

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

yes, I was wondering if we can put up some redirects

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@uri-granta how can we set these redirects up?

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +60 to +65
from trieste.bayesian_optimizer import BayesianOptimizer

bo = BayesianOptimizer(observer, search_space)
num_steps = 15
result = bo.optimize(num_steps, initial_data, model)
query_point, observation, arg_min_idx = result.try_get_optimal_point()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since we stress the ask-tell, perhaps we should use it here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not in the getting started I think. Though you could explicitly link both the expected-improvement and ask-tell tutorials if you want.

Comment on lines +93 to +95
git clone https://github.com/secondmind-labs/trieste.git
cd trieste
pip install -e .
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is incomplete, starting point at best - to do typing, formatting and testing there is a series of additional installation commands...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking they only require tox to run. But yes, the current workflow is a mess and hopefully we'll get a chance to fix (or at least improve) it in not too long.

README.md Show resolved Hide resolved
@@ -1,12 +1,81 @@
# Trieste
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we can add a logo here if we are happy enough with one of the options...

@hstojic hstojic marked this pull request as ready for review February 4, 2023 13:51
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@uri-granta uri-granta left a comment

Choose a reason for hiding this comment

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

Looks great. A few more comments.

.github/workflows/develop-deploy.yaml Outdated Show resolved Hide resolved
.github/workflows/deploy.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Comment on lines +60 to +65
from trieste.bayesian_optimizer import BayesianOptimizer

bo = BayesianOptimizer(observer, search_space)
num_steps = 15
result = bo.optimize(num_steps, initial_data, model)
query_point, observation, arg_min_idx = result.try_get_optimal_point()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not in the getting started I think. Though you could explicitly link both the expected-improvement and ask-tell tutorials if you want.

README.md Outdated Show resolved Hide resolved
Comment on lines +93 to +95
git clone https://github.com/secondmind-labs/trieste.git
cd trieste
pip install -e .
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly speaking they only require tox to run. But yes, the current workflow is a mess and hopefully we'll get a chance to fix (or at least improve) it in not too long.

@hstojic hstojic merged commit da6be1c into develop Mar 3, 2023
@hstojic hstojic deleted the hstojic/update_readme branch March 3, 2023 12:25
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.

2 participants