Skip to content
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/improve offset transformer #522

Merged
merged 32 commits into from Jun 1, 2019

Conversation

@uvchik
Copy link
Member

commented Sep 18, 2018

  • Docstring was wrong. Dictionary was described but list/tuple is used. Is tuple a good idea or would it be better to use to parameters?

  • Sequences was not used, so it was not possible to pass a scalar

  • Improve the documentation. Describe the coefficients was an image(?).

  • Create an example for the example repository?

  • Add equations to docstring

  • Add diagram with three slopes into documentation (no intersect, intersect, with/without minrange)

  • Add constraint test (@uvchik gets combinations)

  • Create a warning if the y-intersect is positive and no min range is defined

  • Add test (@ckaldemeyer could deliver an application for a heat pump and @mloenneberga for a diesel generator?)

@uvchik uvchik requested a review from ckaldemeyer Sep 18, 2018

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2018

@mloenneberga Did you use the OffsetTransformer? Do you have an example?

@uvchik uvchik assigned uvchik and ckaldemeyer and unassigned ckaldemeyer Sep 18, 2018

@coveralls

This comment has been minimized.

Copy link

commented Sep 18, 2018

Coverage Status

Coverage increased (+0.09%) to 92.797% when pulling cf8f6e1 on features/improve_offset_transformer into 2eb4783 on releases/v0_3_0.

@uvchik uvchik added this to the v0.3.0 milestone Oct 26, 2018

@uvchik uvchik added this to Progress in Release of v0.3.0 Nov 7, 2018

@joroeder joroeder changed the base branch from dev to releases/v0_3_0 May 14, 2019

@ckaldemeyer ckaldemeyer removed their request for review May 15, 2019

@ckaldemeyer

This comment has been minimized.

Copy link
Member

commented May 15, 2019

Hi guys! Thanks for this feature but I am not able to contribute at the moment. But keep on going ;-)

@uvchik uvchik assigned joroeder and unassigned ckaldemeyer May 16, 2019

@joroeder joroeder referenced this pull request May 21, 2019
@ckaldemeyer ckaldemeyer referenced this pull request May 21, 2019
2 of 2 tasks complete

joroeder and others added some commits May 21, 2019

@uvchik

This comment has been minimized.

Copy link
Member Author

commented May 25, 2019

Both errors in the init function of the OffsetTransformer class are untested. The tests need to be added.

How to test exceptions: https://stackoverflow.com/a/40638805

Contact me if you have questions.

def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        self.coefficients = [
            solph_sequence(i) for i in kwargs.get('coefficients')]

        for k, v in self.inputs.items():
            if not v.nonconvex:
                raise TypeError('Input flows must be of type NonConvexFlow!')

        if len(self.inputs) > 1 or len(self.outputs) > 1:
            raise ValueError("Component `OffsetTransformer` must not have" +
                             "more than 1 input and 1 output!")

@uvchik uvchik requested review from p-snft, simnh and mloenneberga May 31, 2019

@uvchik

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

I think the component looks pretty good now. The tests are fine the docstring is good and the documentation readable. One can always do more but this should be enough for the release.

Thank you @joroeder for your contribution ❗️

@uvchik

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

I would wait for one approval.

@p-snft

p-snft approved these changes May 31, 2019

Copy link
Member

left a comment

Looks good. (I really like the realistic example in the documentation.)

@uvchik uvchik merged commit 6d3d621 into releases/v0_3_0 Jun 1, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 92.797%
Details

Release of v0.3.0 automation moved this from Progress to Done Jun 1, 2019

@uvchik uvchik deleted the features/improve_offset_transformer branch Jun 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
5 participants
You can’t perform that action at this time.