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

Features/add transformer #351

Merged
merged 23 commits into from Oct 25, 2017

Conversation

Projects
None yet
3 participants
@uvchik
Copy link
Member

uvchik commented Oct 19, 2017

  • Shall we keep the TransformerN1 and Transformer1N?
  • How do we want to define the conversion factors (see below)?

I replaced the LinearTransformer class by a Transformer class, while the new class can handle multiple in- and outputs. The API for all Nx1 and 1xN case is still the same. Furthermore you can combine it to NxM.

The test fail manly because of the new nomenclature. The csv-Reader fails and I don't know why. One problem already exist and is not caused by these changes (see #350).

I kept the equations to keep it compatible with the old N1 and 1N transformer but I wonder why we decided to define the conversion factors for the input flows like this
inflow * conv_factor_inflow = outflow rather than like this inflow = outflow * conv_factorinflow. In the example below this would lead to 0.8 and 0.2 instead of 1.25 and 5. In the end it makes no big difference but we should decide it now.

    bgas = solph.Bus(label="natural_gas")
    bcoal = solph.Bus(label="hard_coal")
    bel = solph.Bus(label="electricity")
    bheat = solph.Bus(label="heat")

    solph.Transformer(
        label="pp_gas",
        inputs={bgas: solph.Flow(), bcoal: solph.Flow()},
        outputs={bel: solph.Flow(), bheat: solph.Flow()},
        conversion_factors={bel: 0.3, bheat: 0.5,
                            bgas: 1.25, bcoal: 5})

@uvchik uvchik added the enhancement label Oct 19, 2017

@uvchik uvchik added this to the v0.2.0 milestone Oct 19, 2017

@uvchik uvchik self-assigned this Oct 19, 2017

@uvchik uvchik requested review from simnh and ckaldemeyer Oct 19, 2017

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 20, 2017

Shall we keep the TransformerN1 and Transformer1N?

As we are breaking the API anyway and I like the idea "There should only be one way to do it." I would vote for only keeping one Transformer class. Otherwise we have very different examples, subclass from three different classes, ...

I think it is of course more effort to adjust everything but worth it!

How do we want to define the conversion factors (see below)?

To me outflow = inflow * conv_factor_outflow is the most intuitive e.g. P_el=Q_b*Eta_el.

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 20, 2017

Thanks for doing this! Meanwhile I really like the idea of only keeping one Transformer as it is very straight and simple.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 20, 2017

To me outflow = inflow * conv_factor_outflow is the most intuitive e.g. P_el=Q_b*Eta_el.

It's pretty obvious this way. But for the inflow it is more ambitious because you cannot call it eta. In a heat pump it is called COP and in a mixed fuel boiler I would call it fraction.

In general this means:

INflow * conversion_factor_OUTflow = OUTflow * conversion_factor_INflow

This will break the API a little more because by now the TransformerN1 ist implemented like this:

OUTflow = INflow * conversion_factor_INflow.

We have to point this out in the release notes, but I think it is clearer this way.

Something like this?

"As the LinearTransformer is depreciated you have to use the Transformer class instead. Please note that the definition of the conversion factors has changed. If you have used the LinearTransformerN1 by now you have to change the class name from LinearTransformerN1 to Transformer and you have to change the conversion_factors from n to 1/n (e.g. from bus1: 2 to bus1: 0.5) !"

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 20, 2017

Something like this?

"As the LinearTransformer is depreciated you have to use the Transformer class instead. Please note that the definition of the conversion factors has changed. If you have used the LinearTransformerN1 by now you have to change the class name from LinearTransformerN1 to Transformer and you have to change the conversion_factors from n to 1/n (e.g. from bus1: 2 to bus1: 0.5) !"

Why should we keep the old ones with a warning? Let's just remove it and change it in all examples, the release nodes and the documentation!?

So we break the API but tell everybody..

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 20, 2017

To me outflow = inflow * conv_factor_outflow is the most intuitive e.g. P_el=Q_b*Eta_el.
It's pretty obvious this way. But for the inflow it is more ambitious because you cannot call it eta. In a heat pump it is called COP and in a mixed fuel boiler I would call it fraction.

In general this means:

INflow * conversion_factor_OUTflow = OUTflow * conversion_factor_INflow

This will break the API a little more because by now the TransformerN1 ist implemented like this:

OUTflow = INflow * conversion_factor_INflow.

We have to point this out in the release notes, but I think it is clearer this way.

I think one can argument in every direction. As long as it is well documented, it's o.k. I guess 😄

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Oct 20, 2017

"As the LinearTransformer is depreciated you have to use the Transformer class instead. Please note that the definition of the conversion factors has changed. If you have used the LinearTransformerN1 by now you have to change the class name from LinearTransformerN1 to Transformer and you have to change the conversion_factors from n to 1/n (e.g. from bus1: 2 to bus1: 0.5) !"

I would skip the last part of the explanation of how to do it now and refer to the new documentation and docstring. Because i do not really get it right now.

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Oct 20, 2017

definitely remove old ones. From my perspectives we should be as rigorous as possible with throwing out stuff with v0.2...the rest just creates overhead (maintenance, user is confused on what to use, blocks further developments due to incompatibility)

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 20, 2017

definitely remove old ones. From my perspectives we should be as rigorous as possible with throwing out stuff with v0.2...the rest just creates overhead (maintenance, user is confused on what to use, blocks further developments due to incompatibility)

👍

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 20, 2017

I would skip the last part of the explanation of how to do it now and refer to the new documentation and docstring.

👍

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 20, 2017

Thank you for you feedback.

  1. The Transformer class replaces LinearTransformer etc. (old stuff will be removed)
  2. The equation looks like this:
    flow(i)/conv_factor(i) = flow(o)/conv_factor(o) with o of outflows and i of inflows
  3. Examples and test are adapted and a new test and a new doctest are created.

The last thing I will do is to create a minimal example but I could merge this one soon to avoid merge conflicts in the future as it effects a lot of modules and add the example in a new branch.

@ckaldemeyer , @simnh @gnn Shall I merge now or do you need more time to review?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 20, 2017

...furthermore I have to adapt the documentation but I still think we should merge soon. I could adapt the documentation together with the minimal example in a new branch.

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 21, 2017

Thank you for you feedback.

The Transformer class replaces LinearTransformer etc. (old stuff will be removed)
The equation looks like this:
flow(i)/conv_factor(i) = flow(o)/conv_factor(o) with o of outflows and i of inflows
Examples and test are adapted and a new test and a new doctest are created.
The last thing I will do is to create a minimal example but I could merge this one soon to avoid merge conflicts in the future as it effects a lot of modules and add the example in a new branch.

@ckaldemeyer , @simnh @gnn Shall I merge now or do you need more time to review?

If tests are running well I would merge. Very cool! Thanks for your effort 😄

uvchik added some commits Oct 21, 2017

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 23, 2017

equation:
http://oemof.readthedocs.io/en/features-add_transformer/api/oemof.solph.html#oemof.solph.blocks.Transformer

network
http://oemof.readthedocs.io/en/features-add_transformer/api/oemof.solph.html#oemof.solph.network.Transformer

documentation
http://oemof.readthedocs.io/en/features-add_transformer/oemof_solph.html#transformer

Thanks a lot.

network
http://oemof.readthedocs.io/en/features-add_transformer/api/oemof.solph.html#oemof.solph.network.Transformer

I just discovered that some code is not formatted to be aligned as example:

Defining an linear transformer: >>> from oemof import solph >>> bgas = solph.Bus(label=”natural_gas”) >>> bcoal = solph.Bus(label=”hard_coal”) >>> bel = solph.Bus(label=”electricity”) >>> bheat = solph.Bus(label=”heat”)

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 23, 2017

I just discovered that some code is not formatted to be aligned as example:

Fixed it.

@ckaldemeyer
Copy link
Member

ckaldemeyer left a comment

Looks good to me. Thanks!

@ckaldemeyer

This comment has been minimized.

Copy link
Member

ckaldemeyer commented Oct 23, 2017

Looks good to me. Thanks!

Sorry, one more suggestion (I should have looked at it before):

b_gas = solph.Bus(label='natural_gas')
b_coal = solph.Bus(label='hard_coal')
b_el = solph.Bus(label='electricity')
b_th = solph.Bus(label='heat')

solph.LinearTransformer(
    label='pp_chp',
    inputs={b_gas: Flow()},
    outputs={b_el: Flow(nominal_value=30),
             b_th: Flow(nominal_value=40)},
    conversion_factors={b_el: 0.3, b_th: 0.4,
                        b_coal: 0.7, b_gas: 0.3})

I think one input is missing here, isn't it?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 24, 2017

Sorry, one more suggestion (I should have looked at it before):

Fixed. Thank you.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 24, 2017

@simnh, @gnn May I merge? I would like to get one more reply.

@uvchik uvchik requested a review from gnn Oct 25, 2017

@uvchik uvchik referenced this pull request Oct 25, 2017

Closed

Roadmap to 0.2.0 #327

15 of 15 tasks complete
@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Oct 25, 2017

I talked to @simnh aside a web conference and orally approved this PR.

@uvchik uvchik merged commit 053c60e into dev Oct 25, 2017

1 check passed

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

@uvchik uvchik deleted the features/add_transformer branch Oct 25, 2017

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