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

Add a transformer with one input and two outputs and a variable fraction between output flows #248

Merged
merged 35 commits into from Mar 9, 2017

Conversation

Projects
None yet
3 participants
@uvchik
Copy link
Member

uvchik commented Nov 11, 2016

In contrast to the simple transformer this transformer will have a variable fraction between its output flows. As there are (so far) no use case for transformers that have a variable fraction and more than two outputs this transformer class will be restricted to one input and two outputs.

The name of the class could be VariableFractionTransformer (to be discussed)

  • add VariableFractionTransformer class
  • add block for VariableFractionTransformer
  • add grouping for VariableFractionTransformer
  • update whatsnew
  • write test
  • write example
  • write documentation (API, solph, example)
  • all tests still work (including example test)

uvchik added some commits Nov 11, 2016

add FlexibleFractionTransformer to solph
The FlexibleFractionTransformer is (by now) limited to one input and two
outputs. The fraction between these Flows is variable.

@uvchik uvchik force-pushed the features/variable_fraction_transformer branch from cc3dba9 to 0a33e11 Nov 21, 2016

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Nov 22, 2016

We should check if the internal calculation of parameters also works with BinaryFlow objects...

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 22, 2016

We do not need the internal calculation. We can move it to the constraint if it does not work. Will you check it.

I am almost done if everything works.

@uvchik uvchik added the enhancement label Nov 22, 2016

@uvchik uvchik added this to the v0.1.2 milestone Nov 22, 2016

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 23, 2016

Anything missing?

@simnh What about your BinaryFlow problem?

@uvchik uvchik removed their assignment Nov 23, 2016

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 23, 2016

We may want to rename the efficiency_condensing parameter.

  • max_conversion_factor
  • conversion_factor_one_output
  • conversion_factor_single_output

@uvchik uvchik self-assigned this Nov 23, 2016

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Nov 23, 2016

I can try to look at the problem regarding binary things later..hopefully..if the example is up, what I think it is.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 23, 2016

I renamed the efficiency_condensing to conversion_factor_single_flow.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 23, 2016

I think I am done so far.

I did not use any special attributes (related to #253) but only the self.conversion_factor_single_flow.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Nov 25, 2016

@simnh If you are satisfied you can merge.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Dec 13, 2016

I am not sure, but I think it should work...Maybe the docstring should contain some infos on limitations, e.g. that the nominal_value of the flow should be set via the inflow and not on the outflows, and thus the binary attributes should also be set on this flow working.

@simnh simnh force-pushed the features/variable_fraction_transformer branch from 24afb25 to 6523be4 Dec 13, 2016

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Dec 13, 2016

Checkout the features/variable_fraction_transformerbranch and run the variable_chp example. Does it work for you? If you have time you can have a look at the documentation.

http://oemof.readthedocs.io/en/features-variable_fraction_transformer/oemof_solph.html#variablefractiontransformer

http://oemof.readthedocs.io/en/features-variable_fraction_transformer/api/oemof.solph.html#oemof.solph.network.VariableFractionTransformer

First of all, thanks for implementing this functionality!

From my point of view the documentation could be improved a bit as it is quite text-heavy and even for me (as I know the functionality) hard to understand.

I would offer to look after some texttual formulations. Additionally, I think it could be helpful to provide some sort of visualisation such as (http://pasteboard.co/9huBf6bKt.png) but in an abstract form, not specificly for power and heat. I think this would make things a bit clearer..

What do you think?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Dec 14, 2016

@ckaldemeyer You are right, this would definitly improve the documentation. For the TwoInputsOneOutput transformer (old solph) I added a picture like the one below. But this picture needs to be revised as the naming is too far from the real parameters. I think this should be a new PR where we create a template that we can use it with all components.

oemof_transformer

@simnh It is always good to make a documentation as clear as possible but it is not forbidden to define the nominal_value for the output flows. If people do not understand what they are doing they should keep with the example.

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Dec 14, 2016

@ckaldemeyer You are right, this would definitly improve the documentation. For the TwoInputsOneOutput transformer (old solph) I added a picture like the one below. But this picture needs to be revised as the naming is too far from the real parameters.

I think 2D or 3D visualisations in coordinate systems such as in this or this arcticle are quite standard for LP/MILP and I would suggest that we use a similar approach.

I think this should be a new PR where we create a template that we can use it with all components.

I agree. It makes sense to use a uniform way to decribe equations!

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Dec 14, 2016

I have pushed my revision of the rst-paragraph/docstring. If you don't like it or if I got something wrong, just change/remove it..

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Dec 15, 2016

I think 2D or 3D visualisations in coordinate systems such as in this or this arcticle are quite standard for LP/MILP and I would suggest that we use a similar approach.

I can also make a suggestion. But after christmas.. ;-)

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Mar 2, 2017

I would like to merge it until v0.1.2. Any serious objections beside optional improvements?

@oemof/oemof-solph The name is VariableFractionTransformer. Any suggestion for improvement?

uvchik added some commits Mar 9, 2017

Merge branch 'dev' of github.com:oemof/oemof into features/variable_f…
…raction_transformer

Conflicts:
	oemof/solph/blocks.py
	oemof/solph/network.py

@uvchik uvchik merged commit 27383b6 into dev Mar 9, 2017

1 check passed

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

@uvchik uvchik deleted the features/variable_fraction_transformer branch Mar 9, 2017

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