ACCESS-NRI / amami

Apache License 2.0
0 stars 0 forks source link

Refactor stash_utils submodule #22

Closed truth-quark closed 1 month ago

truth-quark commented 1 month ago

The Stash class in stash_utils.py adds another object layer for accessing the ATM_STASHLIST lookup. This has a few problems:

Ideally, Stash should be refactored to a dict for simplification & provide a common Pythonic interface. Each code lookup should return an existing ATM_STASHLIST entry, negating the need to create a new Stash record object.

Tasks:

More ideas (see #33):

atteggiani commented 1 month ago

Refactor Stash class & ATM_STASHLIST to dict lookup (avoid Stash obj creation to convert/look up codes)

ATM_STASHLIST is already a dictionary. How do you want to refactor it? I haven't well understood the picture of the new structure you have in mind.

  • Implement custom __getitem__, __setitem__ to work with str, stash codes etc

In the point below you mentioned removing the Stash class. Where do you want to implement the custom __getitem__ and __setitem__ methods?

  • Remove existing Stash class, use dict interface for standard python interoperability

Not sure why you want to remove the Stash class. I believe having a STASH object is useful in many different subcommands (future ones as well).

  • Move lookup to um_utils module as it's part of unified model code

We could move the Stash class to the um_utils module.

But I still haven't really understood the structure of such 'lookup' you refer to.

  • Delete the stash_utils module

This can be done If we move the Stash class to the um_utils module.

truth-quark commented 1 month ago

Refactor Stash class & ATM_STASHLIST to dict lookup (avoid Stash obj creation to convert/look up codes)

ATM_STASHLIST is already a dictionary. How do you want to refactor it? I haven't well understood the picture of the new structure you have in mind.

  • Implement custom __getitem__, __setitem__ to work with str, stash codes etc

In the point below you mentioned removing the Stash class. Where do you want to implement the custom __getitem__ and __setitem__ methods?

Ooops, I mucked up the original ticket text. This should be fixed, explaining refactoring the Stash class into a customised dict for the ATM list.

I'm currently working on an example of the above work in another branch...

atteggiani commented 1 month ago

Captured refactoring of stash_utils in #20

truth-quark commented 1 month ago

I'll reopen as it covers refactoring the custom Stashclass (which needs some tweaks to bring in line with Python conventions).

atteggiani commented 1 month ago

@truth-quark Please edit the body of this issue:

truth-quark commented 1 month ago

I've updated the issue/crossed out the structural changes from #33.

The class modifications need more thought, so I'll update that after another code experiment.