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

Remove fixed_costs attribute from Flow class #407

Merged
merged 15 commits into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@uvchik
Copy link
Member

uvchik commented Dec 12, 2017

As this attribute is not necessarily needed but might be confusing in investment models we would like to remove it. This came up in PR #396 but I think it is cleaner to create a separate PR.

@oemof/oemof-solph Any objections?

@uvchik uvchik added this to the v0.2.0 milestone Dec 12, 2017

@uvchik uvchik self-assigned this Dec 12, 2017

@uvchik uvchik requested review from simnh and ckaldemeyer Dec 12, 2017

@uvchik uvchik referenced this pull request Dec 12, 2017

Merged

Add investment limit #396

3 of 3 tasks complete
@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 12, 2017

I would raise an error if fixed_costs are still used. Because of **kwargs additional keyword arguments are allowed and people may not notice the removal of this attribute.

@c-moeller

This comment has been minimized.

Copy link
Member

c-moeller commented Dec 12, 2017

Although I do not fully understand the problem with fixed costs I would be ok.

But I do not understand the hurry. Why do we need that for v0.2? How is the communication to the developers who commited to code adaptions in the example files? Do they need to have this issue in mind?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 12, 2017

But I do not understand the hurry.

According to the semantic versioning rules we are only allowed to change the API in mayor releases. In our cases this is v0.2, v0.3... That's why we have some last minute discussions about API changes right now.

Do they need to have this issue in mind?

I think removing fixed_costs takes about one minute so that should not be the point.

@mloenneberga mloenneberga referenced this pull request Dec 13, 2017

Closed

Upcoming release v0.2.0 #382

46 of 50 tasks complete
self.fixed_costs = Expression(expr=fixed_costs)

return fixed_costs
return 0

This comment has been minimized.

@ckaldemeyer

ckaldemeyer Dec 13, 2017

Member

If you agree, we could also remove the objective expression completely as it holds nothing!?

This comment has been minimized.

@ckaldemeyer

ckaldemeyer Dec 13, 2017

Member

It's your decision @uvchik. For the rest, I am fine. Thanks!

This comment has been minimized.

@simnh

simnh Dec 13, 2017

Member

@ckaldemeyer you can remove the 'empty' method, just make sure that no error occurs, as for all blocks the method is automatically called

This comment has been minimized.

@ckaldemeyer

ckaldemeyer Dec 13, 2017

Member

@ckaldemeyer you can remove the 'empty' method, just make sure that no error occurs, as for all blocks the method is automatically called

Let's wait what @uvchik says. But I can do it!

@ckaldemeyer
Copy link
Member

ckaldemeyer left a comment

Thanks!

@uvchik uvchik assigned simnh and unassigned uvchik Dec 13, 2017

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 13, 2017

I just moved this stuff from one branch to another to make it more transparent. I actually did not start these changes.

In the end we have to search for all occurrences of fixed_costs anywhere in the code and the documentation and remove it. If all tests work and our test are robust we will not have any problems.

I can help but do not feel responsible for the whole PR.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 13, 2017

Because of the kwargs nothing will happen if people define fixed_costs. They will be silently ignored by solph. So people may not notice that we removed fixed_cost, keep on using them and wonder why the results are different.

@oemof/oemof-developer-group What shall we do about it? Warning? Error?

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 13, 2017

A created a branch in oemof_examples repository with the same name where I removed all the fixed costs. Somebody might review this on the pull request:

oemof/oemof-examples#9

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 13, 2017

I wonder why all test pass... :-/. But I looked at the test and (un?fortunately) the fixed_cost attribute did not effect the tests...

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 13, 2017

You forgot one comma in line 56 in test_components.py.

If the test is not about the fixed_costs attribute it should not fail if you remove it. So everything is fine. The constraint test failed but I fixed them.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 13, 2017

But you did not push this, right? :)

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 13, 2017

Travis-Ci test will merge and then test. As I added test for the meta results the objective function was tested and the objective function will be different if fixed_costs are removed.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 14, 2017

Thanks @uvchik! So what about can we merge?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

Could we first answer the question above.

Because of the kwargs nothing...

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 14, 2017

I added an attribute error...hope this is ok

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

Normally warnings are used if you do not want the program to stop.

msg = "The `fixed_costs` attribute has been removed with v0.2!"
warnings.simplefilter('always', DeprecationWarning)
warnings.warn(msg, DeprecationWarning)

According to the python documentation the depreciated warning is ignored by default. Therefore you have to change the filter or use a different warning (e.g. SyntaxWarning).

If you raise a warning you could also raise an error.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 14, 2017

For me a depreciated warning is fine (and if the program stops). If you want to change it feel free, otherwise I would merge this one?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

I just wonder because it is not in line with common practise.

Your first variant (32c1ebb) was fine I just wonder why you changed it because now it is not in line with common practise.

Warning messages are typically issued in situations where it is useful to alert the user of some condition in a program, where that condition (normally) doesn’t warrant raising an exception and terminating the program.[1]

[1] https://docs.python.org/3/library/warnings.html#module-warnings

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 14, 2017

Ok then lets revert it...

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

I will write a small test. Wait with you merge....

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

Okay, done.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

Approve (but I am not a reviewer) 😄

The error message could contain an advise but we can merge it and anybody who wants could change it.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2017

@simnh According to yesterdays talk: 851a4d7, simple but effective.

@simnh

simnh approved these changes Dec 14, 2017

@simnh simnh merged commit 6d3cd5a into dev Dec 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@simnh simnh deleted the revision/remove_fix_costs branch Dec 14, 2017

@jnnr jnnr referenced this pull request Jan 23, 2019

Open

Fix Cost Implementation #556

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment