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

Improve performances #905

Merged
merged 4 commits into from
Sep 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

### 34.4.2 [#905](https://github.com/openfisca/openfisca-core/pull/905)

- Fix minor errors that result in operations being uselessly repeated over and over.

### 34.4.1 [#904](https://github.com/openfisca/openfisca-core/pull/904)

- Improve performance graph introduced in 34.4.0
Expand Down
11 changes: 6 additions & 5 deletions openfisca_core/periods.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

Since a period is a triple it can be used as a dictionary key.
"""

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to remove the type annotations if considered off-topic. I've added them on the way while working on other optimizations I since then reverted, but figured out they are valuable and don't complicate the review that much.

from __future__ import annotations

import calendar
import datetime
import re
Expand Down Expand Up @@ -567,12 +570,10 @@ def offset(self, offset, unit = None):
"""
return self.__class__((self[0], self[1].offset(offset, self[0] if unit is None else unit), self[2]))

def contains(self, other):
def contains(self, other: Period) -> bool:
"""
Returns ``True`` if the period contains ``other``. For instance, ``period(2015)`` contains ``period(2015-01)``
"""
if not isinstance(other, Period):
other = period(other)
return self.start <= other.start and self.stop >= other.stop

@property
Expand Down Expand Up @@ -619,7 +620,7 @@ def size_in_days(self):
raise ValueError("Cannot calculate number of days in {0}".format(unit))

@property
def start(self):
def start(self) -> Instant:
"""Return the first day of the period as an Instant instance.

>>> period('month', '2012-2-29', 4).start
Expand All @@ -628,7 +629,7 @@ def start(self):
return self[1]

@property
def stop(self):
def stop(self) -> Instant:
"""Return the last day of the period as an Instant instance.

>>> period('year', 2014).stop
Expand Down
2 changes: 1 addition & 1 deletion openfisca_core/populations.py
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ def ordered_members_map(self):
This function only caches the map value, to see what the map is used for, see value_nth_person method.
"""
if self._ordered_members_map is None:
return np.argsort(self.members_entity_id)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The value was never put in cache, resulting in running this costly argsort many many times

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to reset the cache to None in @members_entity_id.setter.

self._ordered_members_map = np.argsort(self.members_entity_id)
return self._ordered_members_map

def get_role(self, role_name):
Expand Down
1 change: 1 addition & 0 deletions openfisca_core/simulations.py
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ def purge_cache_of_invalid_values(self):
for (_name, _period) in self.invalidated_caches:
holder = self.get_holder(_name)
holder.delete_arrays(_period)
self.invalidated_caches = set()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once all the invalidated items have been removed from the cache, we can empty the set, otherwise it will keep trying to remove them from cache over and over.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!


def calculate_add(self, variable_name, period):
variable = self.tax_benefit_system.get_variable(variable_name, check_existence = True)
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

setup(
name = 'OpenFisca-Core',
version = '34.4.1',
version = '34.4.2',
author = 'OpenFisca Team',
author_email = 'contact@openfisca.org',
classifiers = [
Expand Down