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

Several performance increases #302

Merged
merged 10 commits into from
Jan 30, 2024
Merged

Several performance increases #302

merged 10 commits into from
Jan 30, 2024

Conversation

HLWeil
Copy link
Member

@HLWeil HLWeil commented Jan 26, 2024

Improve speed of investigation file writer

Writing investigation file with many rows (e.g. 5000 studies) was very slow with quadratic time. Fixed by using the improved rowWithRange function form FsSpreadsheet, which skips checking all rows on every call.

Improve speed of Reading

  • Improved fillMissing by roughly a factor of 8

image

  • Improved addRows, which had a quadratic time dependency because of using getRowCount in every step
    image

@HLWeil HLWeil changed the title Improve speed of investigation file writer Several performance increases Jan 29, 2024
let expected = 1000 // this is too high and should be reduced

Expect.equal (wb.GetWorksheets().Count) 1 "Worksheet count"
Expect.isTrue (runtime <= expected) $"Expected conversion to be finished in under {expected}, but it took {runtime}"
Copy link
Collaborator

@Freymaurer Freymaurer Jan 30, 2024

Choose a reason for hiding this comment

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

can you abstract this as Expect.isFasterThan: (func: unit -> unit) (time: int), which runs both functions and fails if func is slower than time?
Would be much appreciated and could be added to Pyxpecto

EDIT: Maybe think of a fitting name (Expecto might have something like this) for comparing to functions against each other.

@HLWeil HLWeil merged commit 4d6de35 into main Jan 30, 2024
2 checks passed
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.

2 participants