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

MAINT documentation #228

Merged
merged 40 commits into from Jun 15, 2017
Merged

MAINT documentation #228

merged 40 commits into from Jun 15, 2017

Conversation

shukon
Copy link
Collaborator

@shukon shukon commented Apr 16, 2017

Rewriting documentation in general.

  • replacing example leading-ones with svm
  • add options-overview
  • refactor navigation structure (separating manual and quickstart)

This is not to be merged right away, but I would greatly appreciate an opinion on the work so far to see whether it is going in the right direction. Also to know things you are missing in the documentation in general would help.

To read, with sphinx installed:

cd doc/
make html

@codecov-io
Copy link

codecov-io commented Apr 16, 2017

Codecov Report

Merging #228 into development will increase coverage by 0.61%.
The diff coverage is 100%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development    #228      +/-   ##
==============================================
+ Coverage        85.28%   85.9%   +0.61%     
==============================================
  Files               42      42              
  Lines             2270    2369      +99     
==============================================
+ Hits              1936    2035      +99     
  Misses             334     334
Impacted Files Coverage Δ
smac/tae/execute_ta_run_aclib.py 71.21% <ø> (ø) ⬆️
smac/optimizer/local_search.py 95.91% <ø> (ø) ⬆️
smac/utils/io/output_writer.py 96.72% <ø> (ø) ⬆️
smac/tae/execute_ta_run_old.py 78.33% <ø> (ø) ⬆️
smac/epm/random_epm.py 66.66% <ø> (ø) ⬆️
smac/runhistory/runhistory2epm.py 83.33% <ø> (ø) ⬆️
smac/facade/smac_facade.py 94.32% <ø> (ø) ⬆️
smac/epm/base_imputor.py 100% <ø> (ø) ⬆️
smac/epm/rf_with_instances.py 100% <ø> (ø) ⬆️
smac/initial_design/multi_config_initial_design.py 81.08% <ø> (ø) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7319bd6...eff9fbb. Read the comment docs.

@mfeurer
Copy link
Contributor

mfeurer commented Apr 18, 2017

A great addition would be an API documentation, something similar to sklearn's documentation. I'm not sure if this is on your stack and/or whether this is the right time to start this.

@shukon
Copy link
Collaborator Author

shukon commented Apr 18, 2017

@mfeurer I think API documentation would be great and work hand in hand with further improving the docstrings. Sphinx provides automatic generation of documentation from docstrings. However, the current theme does display the generated rst-files nicely. The widely used read-the-docs theme displays the generated API-files rather nicely. I post a screenshot for comparison.

RTD-Theme:
rtd

Bootstrap (current theme):
bootstrap

@mfeurer
Copy link
Contributor

mfeurer commented Apr 18, 2017

Yep that's look fine as well + it's actually developed by something like a company if I understand correctly. What's important to me is that there is a global menu on the page, which isn't there in the default layout.

@shukon
Copy link
Collaborator Author

shukon commented Apr 25, 2017

You can now check out the API that is generated from SMAC as it is right now. Global menu on the left with collapsible per-page-menus.

@mfeurer
Copy link
Contributor

mfeurer commented Apr 25, 2017

I didn't check the text (PING again once that is ready for review), but I like the layout. I wonder though if it makes sense to divide the longer Sections into shorter ones (i.e. manual and quickstart guide).

@mlindauer mlindauer added this to the Release 1.0 milestone Apr 26, 2017
@mlindauer
Copy link
Contributor

@shukon any updates on this PR? Is it ready for a review iteration?

@shukon
Copy link
Collaborator Author

shukon commented May 10, 2017

Soon! I'm on it. Fixed API-generation to get rid of the errors, trying to get toc right and rewrite a couple paragraphs.

@shukon
Copy link
Collaborator Author

shukon commented May 15, 2017

@mlindauer @mfeurer I think this PR is ready for review.

cd doc
make buildapi
make html
  • updates most sections
  • Quickstart provides three examples, branin, svm and spear.
  • The manual so far aims to explain the difference between using SMAC from the cmdline and using SMAC within Python, lists and explains options and file-formats and presents the different TAEs.
  • finally got the API-doc to work properly and tried to update all docstrings so at least the format is processed correctly. I avoided messing with the content too much (because this PR already contains a big amount of changes), but tried to supplement the most important parts.
  • adds F.A.Q. (to which I just added 3 questions I had myself at some point, just thought it might make sense).

@mlindauer
Copy link
Contributor

Hi,
I fixed some minor issues.

Open issues/todos are:

  • developer guide section
    • contributing new code
    • code style
    • very brief overview of architecture
  • the logger output is not consistent in the python and cli example. this is an issue related to the SMAC code and has to be first fixed within SMAC.

@mlindauer
Copy link
Contributor

Another issue I found today is that the API is not build anymore.
The build also throws errors such as
/home/lindauer/git/SMAC3/doc/api.rst:9: WARNING: toctree contains reference to nonexisting document 'apidoc/smac.optimizer'

@mfeurer
Copy link
Contributor

mfeurer commented May 24, 2017

Actually, the first example (using CLI interface to optimize branin) might be intimidating for someone coming from the blackbox optimization literature and is used to an interface like this (see getting started section).

@shukon
Copy link
Collaborator Author

shukon commented May 31, 2017

@mfeurer Added f_min to branin-example (prior the cmd-line explanation). This feels like the interface you talked about. However, I left the cmd-line in, so you have a direct comparison in the same example. What do you think?

@mfeurer
Copy link
Contributor

mfeurer commented Jun 6, 2017

Much better, thanks a lot. In general, it is a good idea to have a very minimal example, such as this one. Maybe it would be great to simplify the f_min example even further?

Copy link
Contributor

@mfeurer mfeurer left a comment

Choose a reason for hiding this comment

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

I went through the manual and a part of the API documentation. However, I stopped the second because a lot of the docstrings are not rendered yet.

:lineno-match:

This way, you can optimize a blackbox-function with minimal effort. However, to
use the whole of *SMACs* capability, a scenario_ object should be used. There are a
Copy link
Contributor

Choose a reason for hiding this comment

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

You're explaining the CLI, so there's no scenario-object here. Maybe it would be a great idea to contrast the three different interfaces of SMAC? But that would be something for a new PR. We should get this one merged ASAP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll keep that in mind.

The usage of *SMAC* from your Python-code is described in the `SVM-example
<quickstart.html#svm-example>`_.
Scenario and PCS are both build within the code. The target algorithm needs to
be registered with a `Target Algorithm Evaluator (TAE) <tae.html#tae>`_, which communicates
Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, the default is to simply pass a function.

doc/options.rst Outdated
The following assumes that the scenario is created via a scenario-file. If it is
generated within custom code, you might not need *algo* or *paramfile*.

Required:
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to dynamically create this list of arguments from the scenario file?

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.

doc/options.rst Outdated
* *tuner-timeout* is the maximum amount of CPU-time used for optimization. Default: inf.
* *wallclock_limit* is the maximum amount of wallclock-time used for optimization. Default: inf.
* *runcount_limit* is the maximum number of algorithm-calls during optimization. Default: inf.
* *minR* is the minimum number of calls per configuration. Default: 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please reword this description? minimum number of calls and maximum number of calls is not very descriptive if a user doesn't know about the intensify procedure.

doc/options.rst Outdated
# Forbiddens:
{parameter_name_1=value_1, ..., parameter_name_N=value_N}

.. note::
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure? Can't it read both the old and the new format? Or do you mean that it is more restrictive than the implementation of the new PCS format in SMAC2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure what I mean ;). I'll cut the note out.

smac\.epm\.random\_epm module
=============================

.. automodule:: smac.epm.random_epm
Copy link
Contributor

Choose a reason for hiding this comment

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

@mlindauer @KEggensperger as a general question, do we want the documentation to show all methods which are inherited from the base class? I think this would be a great addition to the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't mind as long as the documentation stays concise

smac\.epm\.rf\_with\_instances module
=====================================

.. automodule:: smac.epm.rf_with_instances
Copy link
Contributor

Choose a reason for hiding this comment

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

@sfalkner the docstring for rf_with_instances mentions the parameter max_num_nodes. I'm currently wondering if the tree is build in depth-first or breadth-first fashion as this would change the behaviour?

smac\.epm\.rfr\_imputator module
================================

.. automodule:: smac.epm.rfr_imputator
Copy link
Contributor

Choose a reason for hiding this comment

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

@KEggensperger could you please add the return values to the docstring of the impute() function?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just pushed a commit to this branch adding some more documentation to the BaseImputor class. See 0d9c36e

@@ -0,0 +1,7 @@
smac\.facade\.smac\_facade module
=================================

Copy link
Contributor

Choose a reason for hiding this comment

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

@shukon could you please update the return value of get_trajectory in the docstring? This function actually returns a named tuple now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

smac\.intensification\.intensification module
=============================================

.. automodule:: smac.intensification.intensification
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why for most classes the constructor isn't shown?

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, this is intended. It is toggled via this option. Would you prefer it to be shown?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to see all methods of a class. @KEggensperger @mlindauer what's your opinion on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

All methods. What would be a good argument to hide the constructor?

@mfeurer
Copy link
Contributor

mfeurer commented Jun 6, 2017

@shukon also, is there a reason why you reference pages by their file names and don't reference the page by its name?

@shukon
Copy link
Collaborator Author

shukon commented Jun 10, 2017

also, is there a reason why you reference pages by their file names and don't reference the page by its name?

@mfeurer Because the links where not working the way I wanted before - do the work for you? Especially with the "#"-link.

Anymore requested fixes I missed?

@mfeurer
Copy link
Contributor

mfeurer commented Jun 14, 2017

@mlindauer @shukon @KEggensperger I think we should merge this now as this PR does what it's supposed to do: restructure the docs so that the structure is the way we want it to be. Polishing the API documentation is in my opinion better suited in a new PR to not make this PR too big and make it block the development of SMAC. What do you guys think?

Copy link
Contributor

@mlindauer mlindauer left a comment

Choose a reason for hiding this comment

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

I haven't looked into the current version, but if you think that we can merge it now, I'm happy to approve it.

@mfeurer mfeurer merged commit f8c77e9 into development Jun 15, 2017
@mfeurer
Copy link
Contributor

mfeurer commented Jun 15, 2017

I just merged this and opened new issues #265 and #266.

@mfeurer mfeurer deleted the doc/restructure_examples branch June 15, 2017 09:19
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

5 participants