Improve annuity calculation in economics tool

[c-moeller]

• Describe your pull request as transparent as possible:
  The annuity function in the economics tool has been supplemented by possible multiple investment and cost decreases. Both parameters ('u', 'cost_decrease') can be set optionally. If not set, the function works as before.

• Related issues:
  The function was suggested by a user on the last user meeting: https://github.com/baumconsult/gridcon/blob/master/economics_BAUM_171221.py
  This is now the output of the working group from the last developer meeting.
  This issue Add economic calculations to oemof oemof-tools#9 might also be related.

[simnh]

Nice! From the source code perspective, it does not adhere to pep8 (or our guidelines)...maybe this could be adapted?

Otherwise I am fine with the improvement

[uvchik]

Thank you for improving this calculations.

1. Could you write a short test similar to this one.
2. I would prefer optional parameters instead of **kwargs. It is much more straightforward.

def annuity(capex, n, wacc, u=None, cost_decrease=0)
    """...."""
    if u is None:
        u = n

[coveralls]

Coverage increased (+0.06%) to 86.971% when pulling b2da53b on features/economics_improv into 6c32099 on dev.

[coveralls]

Coverage increased (+0.06%) to 86.971% when pulling b2da53b on features/economics_improv into 6c32099 on dev.

[coveralls]

Coverage increased (+0.06%) to 86.971% when pulling 7d0c456 on features/economics_improv into 6c32099 on dev.

[lmilletb]

within these last commits I have tried to: improve docstrings for clarity, add tests to cover additional inputs, adequate or get closer to pep8 (don't have much experience, is it now correct? how can it still be improved?), modify kwargs as suggested by @uvchik
I think it is good to merge :)

[lmilletb]

please, could you review logging block? I have seen different criteria for logging along different files. Otherwise, any suggestion on how to improve test coverage by adding an example that checks those logging lines?

[uvchik]

Thank you for the changes. Tests looks good. I just fixed a few pep8 Issues but in general everything was fine.

Sometimes we use logging warnings but the attention level is quite low. So I would rather raise a python warning or even an error if something is wrong.

Warning: The results might be unrealistic due to a strange combination of parameters but in some (rare) cases it might be okay. In that case it has to be possible to switch off the warning because a warning is annoying, if the results are okay.

Error: No chance of getting good results with this combination of input parameters.

In most cases I prefer raising an error or doing nothing.

@lluismillet Does it help?

[lmilletb]

Thank you for the corrections and the tips.
Regarding the logging, if the condition

if ((n < 1) or (wacc < 0 or wacc > 1) or (u < 1) or
          (cost_decrease < 0 or cost_decrease > 1)):

is True, a python error will raise by itself as a division by zero will be encountered within the return clause.
The logging I added was just an additional way to track what went wrong, in case the source of this error might not trivially be understood by the user.
Anyway, in this case the error raising will probably be clear enough, as you suggested @uvchik.
For me, we could delete the logging block and is ready to merge.

[uvchik]

It might help the user so you can keep the if statement but raise an error instead of a logging warning:

if ....:
    raise ValueError("Input arguments for annuity function out of bounds!")

[lmilletb]

Trying to adopt this error raising I figured out I do not have rights to commit on this branch anymore..

[uvchik]

Trying to adopt this error raising I figured out I do not have rights to commit on this branch anymore..

Sure? According to the team lists you should have writing access.

[lmilletb]

Trying to adopt this error raising I figured out I do not have rights to commit on this branch anymore..

Sure? According to the team lists you should have writing access.

sorry, it was just a firewall problem.

[uvchik]

Thanks. I added a test to test the ValueError. I think we can merge now.

[p-snft]

Looks good to me.

[p-snft]

The docstring uses abbreviations for the formulas. I will approve this here, as it is common practice. However, I would advice symbols.