Skip to content

Conversation

@maarten-ic
Copy link
Collaborator

This change slightly improves:

  • The logic, by moving all lazy-loading logic into dedicated subclasses.
  • Memory consumption:
    Structures and arrays of structures no longer store a _lazy boolean attribute. Lazy contexts are only stored when an IDS is lazy loaded.
    Observed 0.5-1.5% memory reduction as measured by pympler.asizeof.asizeof.
  • Performance, by removing if self._lazy checks in (arrays of) structures and initialization of self._lazy. This is hardly noticable in most use cases. Specific use cases, like making a deepcopy of an IDS, can be sped up by a couple percent.

The introduction of new subclasses has some impact on users:

  1. When printing lazy loaded IDS toplevels, structures or structarrays, this will print for example <LazyIDSToplevel (IDS:magnetics)> instead of <IDSToplevel (IDS:magnetics)>. This is actually very useful as it makes it immediately clear that the IDS is lazy loaded! But it's a change that may be unexpected when upgrading to a newer minor version
  2. Downstream users may have built logic that would check if type(xyz) is IDSStructure:. This is not good practice, but this logic would stop working for lazy loaded IDSs. The recommendation is to check if isinstance(xyz, IDSStructure) which keeps working correctly.

Testing succeeded (both unit tests, and the IMAS-Paraview plugins which rely heavily on lazy loading). Also, from a software engineering perspective, this change is useful. However, I had hoped for bigger performance improvements.
Given the potential impact for users, we should discus if we want to include this in an IMAS-Python 2.x release. @olivhoenen what are your thoughts on this?

  • Update developer documentation for lazy loading (and fix corresponding CI failure of the docs)

Slightly improves logic, memory consumption and performance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant