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

Revision/expanding sequences Sequences with default value get not expanded #438

Merged
merged 7 commits into from Feb 2, 2018

Conversation

Projects
None yet
3 participants
@henhuy
Copy link
Contributor

henhuy commented Jan 16, 2018

This revision aims to reduce unnecessary list extensions while creating the model - see related issue #436.

Related issue: #436
Located in: Major change was made to oemof.solph.plumbing
Functionality:
While building the sequence an internal attribute self.default_changed is set to False. Whenever __getitem__ of sequence is called, only the default value is returned. Before, every time an internal list was extended to requested index. Only, if __setitem__ is called attribute self.default_changed is set to True and an internal list is build, as the sequence no longer consists of only one default value.
Outcome:
This revision leads to three things:

  1. Testing run-time decreased! (for me, from 8sec to 5sec!) :)
  2. __len__ function of Sequences with only one default value now returns 0 - this is no problem for all tests, but users looking into solph components may get different results.
  3. I changed param_results-API, so that Sequences containing only the default value are now added to the 'scalars' dictionary.

Not implemented:
The other idea from #436, setting up a list containing entries for every time-step is not implemented and maybe even unnecessary, due to new implementation!

I think these changes are good, improving oemof solph! What do you think?

henhuy added some commits Jan 16, 2018

If sequences hold only one default value, no sequence list is build
By this change unnecessary list extensions are suppressed, leading to
faster setup of Model and less "memory movements".
Adapted processing tests to new param_results
As sequences with only one default value are moved to scalars, this has
to be regarded in the tests.

@henhuy henhuy added this to the v0.2.1 milestone Jan 16, 2018

@henhuy henhuy self-assigned this Jan 16, 2018

@henhuy henhuy requested review from simnh , uvchik and ckaldemeyer Jan 16, 2018

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 16, 2018

As the param_results dict was changed (to the good, I think) this will have implications for #433. If this PR is accepted, I will change related calculations...

@simnh simnh requested a review from gnn Jan 16, 2018

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 16, 2018

I wonder why there are no travis checks for this PR? Can you explain me?

super().__init__(*args)

def __getitem__(self, key):
if not self.default_changed:
return self.default

This comment has been minimized.

@simnh

simnh Jan 16, 2018

Member

Do I understand this right, that no iterable object but a scalar is returned, if not default_changed? I think, it kind of strange to have a sequence class that does not return a iterable object (in some cases). My idea was rather to move this to the flow/node classed instead, where the default values for sequences are defined (min, max, etc...)

@simnh
Copy link
Member

simnh left a comment

Move default check to flow / node objects

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Jan 16, 2018

Sorry, I think I did not read correctly...

Forget everything I said before...I think we can do it this way...but one last thing: plumbing is not in core atm...

@simnh

simnh approved these changes Jan 16, 2018

@uvchik

This comment has been minimized.

Copy link
Member

uvchik commented Jan 16, 2018

I wonder why there are no travis checks for this PR? Can you explain me?

That's strange because the test has been done: https://travis-ci.org/oemof/oemof/builds/329396739

The test failed by the way :-P

The last visible test is 7 days ago. I have no idea.

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 16, 2018

Bit confused @simnh : Is your request still open, or did you change your mind and approved?

Nevertheless I am not sure, whether we should mix the idea of default values with the sequence class..

I don't really see the advantage of creating a variable length list, if solph only needs the default value every time...

The test failed by the way :-P

Sorry, i did not check for the docstring tests (I will change my testing parameters to take these into account for the future). The test fails, because no iterable list is created internally.

So, there is an decision to make, if this behaviour is wanted or not. Personally, I prefer my implementation as oemof is set up faster and I don't see the point in creating a list of 8760 times the same value...
But no offense there - it was just an idea!

henhuy added some commits Jan 16, 2018

Fixed __len__, __setitem__ and __iter__ function of sequence with def…
…ault value

Now, sequence emulates __len__ and __iter__ behaviour of list without
actually implement a list. Highest index is know stored in case a list
is changed at later point. Then, list is initialized with highest known
index.
@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 16, 2018

I fixed sequences len, setitem and iter function in order to emulate typical list behavior without actually creating list internally.
@uvchik Now, also the doctests pass!
Maybe this resolves your last concerns @simnh?

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 16, 2018

What do you mean with plumbing is not in core atm?

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Jan 16, 2018

Is your request still open, or did you change your mind and approved?

I already approved this one, as I changed my mind ;-)

What do you mean with plumbing is not in core atm?

the module is not located on the core level but in the solph package...because you said above 'this will improve oemof core'

@uvchik

uvchik approved these changes Jan 17, 2018

@henhuy

This comment has been minimized.

Copy link
Contributor Author

henhuy commented Jan 17, 2018

plumbing is not in core atm...

Ah okay! (I had to re-read my own text) Thanks for the hint - changed PR description...

@henhuy henhuy removed the request for review from ckaldemeyer Jan 17, 2018

@henhuy henhuy merged commit ae88109 into oemof:dev Feb 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment