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

Asig metadata #15

Open
thomas-hermann opened this issue Oct 5, 2019 · 4 comments
Open

Asig metadata #15

thomas-hermann opened this issue Oct 5, 2019 · 4 comments
Labels
enhancement New feature or request feedback wanted

Comments

@thomas-hermann
Copy link
Member

For Arecorder.record(), it would be useful to store the time stamps when recordings were made. One way would be to change recordings.append((timestamp, asig)), but that would make access and interpretation less intuitive. So that lead me to the idea of Asig.metadata:

I propose to add an attribute Asig.metadata, which could be a dict with fields such as

  • timestamp,
  • author,
  • recording_device,
  • history (which could be a list itself and store those things currently appended to label)
  • etc. (ideas?)

By the way, Asig.label would be a perfect item in the meta data dict!

Before such a far-reaching change, I'd like to hear your feedback and ideas on this issue.

Note that we already have a context variable named '_' (yes, funny name, but short and nice, e.g. to access results of a1.find_events().plot() via a1._['events'] and self._['plot'].
This is convenient for returning auxilliary data without breaking the logic of returning self always.

Hand in hand with that, I feel that the currently often used strategy to return a new Asig with new data

return Asig(newsig, self.sr, self.label, self.cn,...)

is suboptimal as it often fails to copy '_' and would, if metadata should be implemented, become even more complex. So probably a static method Asig._copy() for devs, which generally duplicates self w/o copying sig would help, so instead we could simply write

return Asig._copy(self, newsig, attr_to_overwrite=new_value,...)

Looking forward to hear your thoughts on both issues.

@thomas-hermann thomas-hermann added enhancement New feature or request feedback wanted labels Oct 5, 2019
@thomas-hermann
Copy link
Member Author

thomas-hermann commented Oct 11, 2019

Please take a look at the new branch feature-metadata, which implements a first version of a Metadata class.

  • I moved Asig.sr, Asig.cn, Asig.label to Metadata, (as this is meta data ;-)
  • but I added @properties, so that they can also be accessed and set as we are used to
  • the notebook Asig-metadata-test.ipynb shows how it works
    • the output is still a bit verbose yet...
  • a new Asig._copy_update() method facilitates a deep copy of an asig while excluding the deep copy of the contained sig in case a sig is updated.
    • This will facilitate lots of method returns
    • However, so far only few (fade_out, norm, plot, getitem) are rewritten, mainly for testing.

Pros:

  • metadata are flexible, extensible
  • better handling of revision, as list in metadata
  • simpler compilation of an Asig-to-return, no risk of forgetting to set values
  • context always copied on chained method invocations

Cons:

  • explicit access to new metadata (e.g. author, timestamp, etc.) takes
    asig.md.author (which is less readable than asig.author would be. However, I dislike the idea to clutter the Asig with lots of attributes.

Summary:

  • I am not 100% happy as of now... Ideas/ Suggestions welcome.

Next Steps:

  1. please could you take a look and comment?
  2. improve design,
  3. when we are satified, roll out to all methods

@aleneum
Copy link
Member

aleneum commented Oct 14, 2019

Naming

just a thought. maybe info or meta is more comprehensive as a metadata name... asig.info.timestamp

Metadata object

Metadata could be moved to a separate file to declutter Asig.py. As it is a pure helper class I'd make it less verbose:

import time

class Metadata(dict):
    __getattr__ = dict.get
    __setattr__ = dict.__setitem__
    __delattr__ = dict.__delitem__

    def __repr__(self):
        res = "Asig metadata:\n"
        for attr, value in self.items():  # sorted(self.items()) if keys should be in alphabetical order
            if value:
                if attr == 'timestamp':
                    value = time.strftime("%Y-%m-%d %H:%M:%S", time.localtime(value))
                res += f"   {attr:>15} = {value}\n"
        return res

md = Metadata(foo="bar", bar=1, timestamp=time.time())
md.baz = 2.4
print(md)

In this less verbose form with less than 20 LOC it may also stay in Asig.py.

If possible do not name variables str, input or such alike. This will shadow and sometimes even overload builtins with nasty side effects.

@aleneum
Copy link
Member

aleneum commented Nov 27, 2022

Three years later I'd probably use dataclasses for Metadata. Also we might want to introduce proper typing when we are at it. Python 2 support should not be an issue any longer (or is it)?

@thomas-hermann
Copy link
Member Author

wow. three years it is! tempus fugit.
Yes agreed. Data classes sounds very appropriate. Thanks for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feedback wanted
Projects
None yet
Development

No branches or pull requests

2 participants