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

Review #31

Open
thomaspinder opened this issue Aug 16, 2023 · 1 comment
Open

Review #31

thomaspinder opened this issue Aug 16, 2023 · 1 comment

Comments

@thomaspinder
Copy link

thomaspinder commented Aug 16, 2023

Firstly, I would like to congratulate the developers on putting together such a nice package. The package is of a very high-standard and, in my opinion, contains functionality far beyond the requirements set out by JOSS. I have a few comments and suggestions that I detail below. Note, I would only require points 5 and 7 being done for me to accept, all other points are suggestions that I feel would further improve the package.

I have opened issues in the repo with my specific comments. Some high-level comments

  1. It would be nice to have more supporting text in your notebooks example. This would be helpful for new users trying to onboard into Bayeso. My acceptance is not conditional on this being done.
  2. Having a real world example of BO would be nice. It's always a true test of an algorithm/implementation when it runs smoothly on real world data. My acceptance is not conditional on this being done.
  3. Nice work on your visualisations - they're very helpful and clear.
  4. It would be nice to decouple the need to always save figures in your examples. If I download/clone a new library, I would not want my local disk filling up with images. Maybe this could be made optional or removed altogether?
  5. In line with the JOSS standards, please could you add a contributing file to the repo and documentation?
  6. You may like to have some end-to-end or smoke tests in your package. With all stochasticity fixed through seeds, this ensures that your package runs end-to-end in a consistent manner. Given you already have some notebooks, I do not think this would be too hard to achieve using the approach we adopted in GPJax. My acceptance is not conditional on this being done.
  7. There is no way in Bayeso for users to request features, flag bugs, or suggest improvements; one can only raise a generic issue. I would suggest having some pre-defined issue templates to more easily signpost users.
@jungtaekkim
Copy link
Owner

@thomaspinder We thank you for your thoughtful review!

We have updated our repository by considering your comments.

  1. You can see the update here bf6a7c0.
  2. We think adding a more impactful real-world example takes some time. We did not add it now, but we are going to add a new application of Bayesian optimization in future. As you already knew, we provided some applications on hyperparameter optimization; see examples in examples/06_hpo.
  3. Thank you so much!
  4. We decoupled the need to always save figures. Please see 30d9aa9, 3b67844, and 712294c.
  5. We have added CONTRIBUTING.md; please see 5132996.
  6. Thank you for introducing a great piece for end-to-end tests. You can find end-to-end tests in tests/integration_test.py; see 712294c.
  7. We have added issue templates; please refer to d584a97.

Thank you again.

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

No branches or pull requests

2 participants