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

Remove Segment.from_row method, no longer used #231

Closed
2 tasks
NickleDave opened this issue Mar 6, 2023 · 0 comments
Closed
2 tasks

Remove Segment.from_row method, no longer used #231

NickleDave opened this issue Mar 6, 2023 · 0 comments

Comments

@NickleDave
Copy link
Collaborator

and the leftover logic for it is confusing.

This method was used by the now-removed 'csv' format, see for example here:

segment = Segment.from_row(row=row)

But now we no longer make individual Segments in the 'generic' format (that replaced 'csv').
Instead we make entire Sequences using the from_keyword instead, parsing csv files with pandas to avoid the maintenance work of doing it ourselves:

seq = crowsetta.Sequence.from_keyword(

(We grab a sequence at a time from the dataframe, and then get arrays from each column 'onset_s', 'offset_s', etc., via the values attribute.)

I realized Segment.from_row is no longer used as I was trying to write examples for the Segment docstring as suggested here in the pyOpenSci review: pyOpenSci/software-submission#68 (comment)

We should just remove it and do the more intuitive thing with attrs:

  • use the attrs optional and have these default to None (NoneType, not a string "None")
  • then do the __post_init__ thing where we throw an error if at least one of them is not specified

Dumping a more detailed explanation of issues cause by from_row, from my dev notes:

  • I can't just make an instance by passing in args
  • It throws an error if I only pass in onset_s / offset_s
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Segment.__init__() missing 2 required positional arguments: 'onset_sample' and 'offset_sample'
  • which is first of all confusing because it looks like these are optional, and I believe that was my intent? From the Segment.__init__ method
    onset_s = attr.ib(converter=attr.converters.optional(float_or_None))
    offset_s = attr.ib(converter=attr.converters.optional(float_or_None))
    onset_sample = attr.ib(converter=attr.converters.optional(int_or_None))
    offset_sample = attr.ib(converter=attr.converters.optional(int_or_None))
  • But then these converters are doing something extra weird, looking for a string "None":
def float_or_None(val):
    """converter that casts val to float, or returns None if val is string 'None'"""
    if val == "None":
        return None
    else:
        return float(val)
  • What's actually going on is that these are doing the work of a Segment.from_row method that can handle a row of strings from a csv. See this unit test for an example:
def test_Segment_init_onset_offset_in_seconds_from_row():
    header = ["label", "onset_s", "offset_s", "onset_sample", "offset_sample"]
    row = ["a", "0.123", "0.170", "None", "None"]
    a_segment = Segment.from_row(row=row, header=header)
    for attr in ["label", "onset_s", "offset_s"]:
        assert hasattr(a_segment, attr)
    for attr in ["onset_sample", "offset_sample"]:
        assert getattr(a_segment, attr) is None
NickleDave added a commit that referenced this issue Mar 6, 2023
CLN: Remove unused Segment.from_row method, fix #231
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

No branches or pull requests

1 participant