Skip to content

Efficiency: remove unnecessary copy operation #288

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.

Sign up for GitHub

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

Open
MetRonnie wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Efficiency: remove unnecessary copy operation #288

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 6 additions & 9 deletions metomi/isodatetime/data.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from math import floor
import operator
from typing import (
Any,
Literal,
cast,
overload,
Expand Down Expand Up @@ -1727,16 +1728,12 @@ def _copy(self) -> 'TimePoint':
new_timepoint._time_zone = self._time_zone._copy()
return new_timepoint

def get_props(self) -> list:
def get_props(self) -> list[tuple[str, Any]]:
"""Return the data properties of this TimePoint as a list of tuples."""
props = []
for attr in self.__slots__:
value = getattr(self, attr, None)
if callable(getattr(value, "_copy", None)):
value = value._copy()

@MetRonnie MetRonnie Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I am convinced this copy was unnecessary. Note it's the time zone that was being copied. The only uses of this method:

  • In the comparison function, where no references to the time zone are created
  • In def get_timepoint_properties_from_seconds_since_unix_epoch(), where a local reference to the time zone is created, but not mutated

@oliver-sanders oliver-sanders Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you are right.

The only mutable property returned is the time zone. This is a TimeZone(Duration) object which is __hash__able. It provides no methods that would mutate the object, you would have to meddle with a _ prefixed variable to mess it up.

Seems reasonable.

props.append((attr[1:], value))
return [
# Have sliced attr string to remove leading underscore
return props
(attr[1:], getattr(self, attr, None)) for attr in self.__slots__
]

def __hash__(self) -> int:
if self._truncated:
Expand Down Expand Up @@ -2632,7 +2629,7 @@ def get_timepoint_properties_from_seconds_since_unix_epoch(num_seconds):
"""Translate Unix time into a dict of TimePoint constructor properties."""
properties = dict(
get_timepoint_from_seconds_since_unix_epoch(num_seconds).get_props())
time_zone = properties.pop("time_zone")
time_zone = cast('TimeZone', properties.pop("time_zone"))
properties["time_zone_hour"] = time_zone._hours
properties["time_zone_minute"] = time_zone._minutes
return properties
Expand Down
Loading