oemof-solph icon indicating copy to clipboard operation
oemof-solph copied to clipboard

Adapt docstrings to new template

Open uvchik opened this issue 5 years ago • 18 comments

In issue #544 we developed a new template for the doc string of block classes.

The following classes still have the old docstring design.

@oemof/documentation : Please assign yourself to one of the following components:

  • [ ] block.Flow
  • [x] block.InvestmentFlow
  • [ ] block.Bus (@joroeder )
  • [ ] block.Transformer (@uvchik)
  • [ ] block.NonConvexFlow
  • [ ] component.GenericStorageBlock
  • [ ] component.GenericInvestmentStorageBlock

Good examples are:

  • components.ExtractionTurbineCHPBlock
  • components.GenericCHPBlock

uvchik avatar Jun 04 '19 15:06 uvchik

I am checking the components.GenericStorageBlock and components.GenericInvestmentStorageBlock at the moment.

oakca avatar Jun 13 '19 14:06 oakca

While trying to understand the issues, I just wanted to note down some annotations for those, that implement the template:

  • if there are no variables created, the headline The following variables are created: is omitted (other possibility could be, to integrate also this headline and just write None)
  • it is sometimes not clear (for people like me, that are not familiar with the code), when variables are classified, that they are created. Some are created to be able to write human readable code - to me it was not clear, if this is consistent for each of the components - is this the case?
  • it also seems like the table of variables and parameters replaces the above mentioned headline. If this is the case - maybe an update of the template could be posted in this issue?

stianchris avatar Dec 05 '19 14:12 stianchris

There are few good examples: OffsetTransformerBlock ExtractionTurbnineCHPBlock GenericCHPBlock GenericStorageBlock ->maybe there needs to be still some small adaptions

There has not been written a template yet. This was the aim of #544 , but the status was that there was an agreement, but this has not been written down somewhere in a general form (e.g. at https://oemof.readthedocs.io/en/stable/developing_oemof.html)

The aim of this issue was to change all docstrings which do not have the format yet. So we need some more people writing their names behind the blocks (see above)

joroeder avatar Dec 05 '19 15:12 joroeder

Hey, are you okay with the new appearance of the docstring, which is now shown in the InvestmenFlow? Are there any errors? What is missing? Any further ideas for improvement?

Please see the actual branch of the Investmentflow development:

InvestmentFlow

Since this is the first of the "more basic" blocks with a new docstring, a feedback would be really nice, and kind of "final" decision on docstring style. In the end, I think it is important that the symbols for the variables and parameters, which occur in many docstrings, are the same.

The major changes and suggestion for basis for decision-making (partly already in the so-far-templates of genericCHP, extraction turbine, etc.):

  • no sets: The sets disappear in the conditions for the constraints
  • structure: variables, constraints, parts of the objective, tables of symbols, notes
  • F(t) as variable for the actual flow value and without indices of starting and ending node
  • separate table for variables and parameters at the almost end
  • all Notes at the end to have a more comprehensive appearance

Maybe, on some points, we can leave some freedom, but some need to be fixed ;)

Two things, I wonder about:

  • Within the text, what is the best style for nonconvex=True or nonconvex is True or nonconvex is set to True - without any highlightning.
  • the attributes in the table: complete or short: flows[i, o].investment.minimum or just minimum ?

So, I would be glad to hear from you ... and then, finally, we will have the best docstrings of the world 👯‍♂ 🥇 🎉

joroeder avatar Feb 28 '20 15:02 joroeder

I just noticed that in the "template-docstrings" of ExtractionTurbnineCHPBlock and GenericCHPBlock, the section, which describes the variables, which are defined in the component (The following variables are created:) is missing. Is that on purpose? I think, it would be good to have these section.

Another step towards improvement would be to use the symbols of the formulas already in the respective class for the parameter and in the introduced variables section of the block docstring. In this way, all symbols of parameter and variables appear already above the constraints, and are summarized in the table at the end of the block-docstring.

What do you think about that?

joroeder avatar Mar 02 '20 14:03 joroeder

#636 gives an example of how the symbols could be in the parameter docstring:

oemof.solph.options.Investment oemof.solph.blocks.InvestmentFlow

joroeder avatar Mar 02 '20 16:03 joroeder

https://github.com/oemof/oemof/pull/636#pullrequestreview-373458735

@oemof/documentation What do you think about that?

joroeder avatar Mar 12 '20 20:03 joroeder

Okay, I tried to summarize all open questions about docstring and then everybody can vote by writing his/her name behing the answer. ;) This should work, everybody can edit a comment, right?

Things, which are more or less decided:

  1. No sets: The sets disappear in the conditions for the constraints and variables a) I agree. joroeder, p-snft, uvchik b) I disagree. c) I don't care.

  2. Structure:

    Introduction-Sentence and link to the paramter-class Variables Constraints Parts of the objective function tables notes

    a) I agree. joroeder, p-snft, uvchik b) I disagree. c) I don't care.

Open decisions:

  1. Tables in csv or === syntax or flexible (see #609) a) csv syntax. joroeder b) === syntax. c) We should keep it flexible. d) I don't care. e) not sure yet, but a) xor b). p-snft, uvchik

  2. Two separate tables with symbols for parameter and variables for future components? (see for example link) a) I agree. joroeder b) I disagree. c) We should keep it flexible. uvchik d) I don't care. e) Another option p-snft (see comment below)

Comments: uvchik: In some cases there is only one variable, it is not worth to make a table for it.

  1. Symbols already in the introduction of variables? and change to syntax: * :attr:invest[i, o] - :math:F_{invest} (see link) a) I agree. p-snft b) I agree, but just the symbol (there is the table anyway). joroeder, uvchik c) I disagree. d) I don't care.

  2. For all "generic" components, F(t) as symbol for the variable of flow[i,o,t] (see link) a) I agree. b) I disagree. b)-i) I disagree and prefer P as sysmbol. p-snft, joroeder c) I don't care. uvchik

  3. Notes - as far as possible - at the end of docstring. a) I agree. joroeder, p-snft, uvchik b) I disagree. c) I don't care.

  4. Symbols of parameters in the "parameter" classes of components (Flow, GenericStorage, Investment, ...) (see link) a) I agree. b) I disagree. c) I don't care. joroeder, uvchik

  5. Attributes in the table: complete or short? flows[i[n], n].investment.existing or existing? (In some cases it is important to have the full attribute name) a) Short. b) Long. p-snft, uvchik c) Depends on block, if there is no confusion, short. joroeder d) I don't care.

Please, feel free to adapt the questions and answers, if this is necessary at some point and I got something wrong. You can also add decisions.

Happy voting!

joroeder avatar Mar 12 '20 21:03 joroeder

  1. Agree.
  2. Agree.
  3. It should be either or. When reading the plaintext, I prefer the rst trables. However, editing CSV is much easier.
  4. If 5 is agreed on, I would suggest to have just one table for the (internal) variables, and to give symbols for a parameters handed to the functions already in the list of parameters.
  5. Yes.
  6. As long as we model energy systems, it's still energy per time, so I would stick to power (P).
  7. Notes where they belong. That is typically at the end.
  8. I do not understand the question, but I like the new style.
  9. Long, if it does not disturb the reading flow.

p-snft avatar Mar 13 '20 08:03 p-snft

@p-snft Thanks for your feedback! I transfered your answers.

  1. I do not understand the question, but I like the new style.

In the following classes I integrated the symbols (not all yet) in the parameter docstring (this is what I mean by No. 8):

Flow Investment GenericStorage

Is this what you mean by ...?

  1. [...] and to give symbols for a parameters handed to the functions already in the list of parameters.

As you can see in the Investment class, depending on where the investment is used (flow or storage), the parameters symbols differ.

One pro argument for keeping the parameter table is, that, if we omit the parameter table, you need to check multipe parameter classes to get the meaning of all symbols used in the equations. However, of course, this is kind of duplication, since the docstring of the parameter class is already kind of a table.

joroeder avatar Mar 13 '20 13:03 joroeder

regarding to No. 3: If I have to decide either-or, I go for CSV table syntax. Since we are using latex, the symbols and formulas are not very readable in plaintext anyway. So, it is always clearer to readthedocs. Furthermore, we cannot meet PEP8 at the tables. From my experience, the editing process of the RST format is really annoying and time intense - maybe I am too stupid for that, and somebody can provide some help :)

joroeder avatar Mar 16 '20 14:03 joroeder

regarding No. 3: symbol tables

you can check the different ways of implementation in InvestmentFlow

Uwe (@uvchik ) suggested to use something like # noqa: D501 to abandone the potential PEP8 error, if we are using tables, where we cannot meet pep8 of 79 characters per line. The problem is, this does not work at the moment for docstrings: https://github.com/PyCQA/pydocstyle/issues/417

If we can't comply with pep8 from the start, it doesn't make much sense to have a pep8 checker, right?

However, I found some nice tools to create grid tables (but header works not automatically): https://www.tablesgenerator.com/text_tables

So, since we have the pep8 issues, I suggest to use csv tables.

joroeder avatar Mar 18 '20 15:03 joroeder

Deadline for votes and feedback: 27.03.2020

Vote! Or keep silient for ever! 😉

Actual examples of how the docstring would look like: InvestmentFlow GenericInvestmentStorageBlock

joroeder avatar Mar 18 '20 15:03 joroeder

Voted!

If you have tables in the docstring you cannot break you have to exclude the docstring from F401.

def function_with_table_in_the_docstring():
    """
    docstring with table
    """  # noqa: F401
    pass

uvchik avatar Mar 23 '20 14:03 uvchik

Deadline for votes and feedback: 27.03.2020

Vote! Or keep silient for ever! wink

@joroeder: Sorry, I missed this one. Wrote an entry to my calendar, but last week was too full and I missed it. Did you consider extending the deadline to raise voter participation? I see only 3 voters up there. If you like, I can still add my feedback.

jnnr avatar Apr 01 '20 09:04 jnnr

Hey, yes, I am still interested to hear your feedback :)

joroeder avatar Apr 01 '20 10:04 joroeder

Ok, thanks! I will take some time for that on Friday :)

jnnr avatar Apr 01 '20 11:04 jnnr

Ok, thanks! I will take some time for that on Friday :)

Which Friday :smirk:

uvchik avatar May 07 '20 09:05 uvchik

With the new approach of constraint generating functions, the template is no longer generally usable. I will close this one.

p-snft avatar Dec 09 '22 21:12 p-snft