Skip to content

Remove csv parsing#49

Merged
domerd merged 2 commits intomasterfrom
change/better-parsing
Jan 11, 2021
Merged

Remove csv parsing#49
domerd merged 2 commits intomasterfrom
change/better-parsing

Conversation

@shaharyarn
Copy link
Copy Markdown
Collaborator

No description provided.

@domerd domerd force-pushed the change/better-parsing branch from a067b75 to c4cca09 Compare January 10, 2021 16:48
marine/marine.py Outdated
csv_parsed_output = next(csv.reader(f, delimiter="\t", quotechar='"'), [""])
return [value or None for value in csv_parsed_output]
return (
output[i].decode("utf-8") if output[i] is not None else None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Also str has no decode in python3, so that should probably be bytes anyway

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Discussed above

marine/marine.py Outdated
@staticmethod
def _parse_output(output: str) -> List[Optional[str]]:
def _parse_output(output: List[str], length: int) -> List[Optional[str]]:
# TODO: this is a bottleneck. Find a better way to provide output from the c code
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is this still a bottleneck? How much faster is this code now?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On average the code is 5% faster.
In some cases (in tests without fields) we slowed the test a bit - might be some overhead we are paying for.

@domerd domerd merged commit 6d92afc into master Jan 11, 2021
@tomlegkov tomlegkov deleted the change/better-parsing branch February 5, 2021 18:43
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.

4 participants