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

Increase test coverage of solph.models module #549

Merged

Conversation

@uvchik
Copy link
Member

commented Dec 13, 2018

There are some special conditions in this module and we should remove them or test them.

Furthermore custom.py is removed from the test coverage check, because we announced that it is not necessary to write tests for the custom components but we should warn users about this.

@uvchik uvchik self-assigned this Dec 13, 2018

@uvchik uvchik added the enhancement label Dec 13, 2018

@uvchik uvchik added this to the v0.3.0 milestone Dec 13, 2018

@uvchik uvchik added this to To do in Release of v0.3.0 via automation Dec 13, 2018

@coveralls

This comment has been minimized.

Copy link

commented Dec 13, 2018

Coverage Status

Coverage increased (+3.5%) to 91.763% when pulling 9884d07 on features/increase_test_coverage_of_model_module into 5bcb7d7 on releases/v0_3_0.

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

First thing to discuss in this context:

At the moment a timeincrement of one is set if the time increment wasn't defined correctly. If I have a time increment of 15 minutes but made a mistake the time increment would be set to one in the background and all I get is a logging warning.

To leave the opportunity to create your own time increment I would add the possibility to pass it directly to the model, otherwise solph tries to get it from the timeindex and if this is not possible an error is raised. If you want we could add our own error message but an error should be raised in my opinion.

ACTUAL VERSION:

    def __init__(self, energysystem, **kwargs):
        super().__init__()
        self.name = kwargs.get('name', type(self).__name__)
        self.es = energysystem
        try:
            self.timeincrement = sequence(self.es.timeindex.freq.nanos / 3.6e12)
        except AttributeError:
            logging.warning(
                'Could not get timeincrement from pd.DateTimeIndex! ' +
                'To avoid this warning, make sure the `freq` attribute of ' +
                'your timeindex is not None. Setting timeincrement to 1...')
            self.timeincrement = sequence(1)

MY PROPOSAL:

    def __init__(self, energysystem, **kwargs):
        super().__init__()
        self.name = kwargs.get('name', type(self).__name__)
        self.es = energysystem
        self.timeincrement = kwargs.get(timeincrement, None)

        If self.timeincrement is None:
            self.timeincrement = sequence(self.es.timeindex.freq.nanos / 3.6e12)

The if block could also look like this to make it more clear for the user.

        If self.timeincrement is None:
            try:
                self.timeincrement = sequence(self.es.timeindex.freq.nanos / 3.6e12)
            except AttributeError:
                raise AttributeError(our_message)

I stumbled over this issue while I was thinking about how to test these lines.
@oemof/oemof-solph What do you think?

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

As nobody commented the idea above I pushed a concrete proposal. If nobody objects I will merge.

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

@oemof/oemof-solph See commit 2f6430 to review the changes on the time increment.

@uvchik uvchik requested a review from simnh Jan 14, 2019

@simnh
Copy link
Member

left a comment

Will it still be possible to set a timeindex with length one?

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Jan 15, 2019

Since v0.2.1 it is possible and since PR #519 it is tested. If all tests pass it is possible

The cleanest way is the following:

datetimeindex = pd.date_range('1/1/2012', periods=1, freq='H')
energysystem = EnergySystem(timeindex=datetimeindex)
model = solph.Model(energysystem)

Until now the following code also worked but I do not like it because it is hidden magic.

energysystem = EnergySystem()
model = solph.Model(energysystem)

With commit fb316ca the default timeindex of an EnergySystem object is None and NOT pd.date_range(start=pd.to_datetime('today'), periods=1, freq='H') as it was before.

@uvchik

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2019

@simnh Do have any objections? May you approve this?

uvchik added some commits Jan 28, 2019

Release of v0.3.0 automation moved this from To do to Progress Jan 28, 2019

@simnh

simnh approved these changes Jan 28, 2019

@uvchik uvchik merged commit 44c0602 into releases/v0_3_0 Jan 28, 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 (+3.5%) to 91.763%
Details

Release of v0.3.0 automation moved this from Progress to Done Jan 28, 2019

@uvchik uvchik deleted the features/increase_test_coverage_of_model_module branch Jan 28, 2019

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