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

added engines to profiling as well as engine_test in test_profile #532

Merged
merged 11 commits into from
Jan 15, 2021

Conversation

PaulJonasJost
Copy link
Collaborator

Hello,

i added the engines to the profiling. Currently (until approval) i left the option to use the current version of profiling when using engine=None. I also added a test in test_profiles.

Kind regards,
Paul

@yannikschaelte
Copy link
Member

Thanks @PaulJonasJost , much appreciated!

@codecov-io
Copy link

codecov-io commented Dec 3, 2020

Codecov Report

Merging #532 (0b071dd) into develop (df705ce) will increase coverage by 0.48%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #532      +/-   ##
===========================================
+ Coverage    87.94%   88.43%   +0.48%     
===========================================
  Files           69       71       +2     
  Lines         4405     4443      +38     
===========================================
+ Hits          3874     3929      +55     
+ Misses         531      514      -17     
Impacted Files Coverage Δ
pypesto/profile/profile.py 91.89% <100.00%> (-2.76%) ⬇️
pypesto/profile/task.py 100.00% <100.00%> (ø)
pypesto/profile/walk_along_profile.py 100.00% <100.00%> (ø)
pypesto/objective/amici_calculator.py 90.00% <0.00%> (+1.66%) ⬆️
pypesto/profile/options.py 92.30% <0.00%> (+7.69%) ⬆️
pypesto/engine/multi_process.py 92.30% <0.00%> (+7.69%) ⬆️
pypesto/engine/multi_thread.py 100.00% <0.00%> (+8.00%) ⬆️
pypesto/objective/amici_util.py 83.03% <0.00%> (+8.92%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df705ce...0b071dd. Read the comment docs.

@yannikschaelte
Copy link
Member

@PaulJonasJost sorry, we did not respond overly quickly here. Could you rerun the tests to see whether all works with the latest releases of AMICI and PEtab? Then this should be almost gtg (@jvanhoefer @dilpath ?).

Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

Looks good 👍

pypesto/profile/profile.py Outdated Show resolved Hide resolved
pypesto/profile/task.py Outdated Show resolved Hide resolved
pypesto/profile/profile.py Outdated Show resolved Hide resolved
pypesto/profile/task.py Outdated Show resolved Hide resolved
pypesto/profile/walk_along_profile.py Show resolved Hide resolved

# check results
for count, _engine in enumerate(engines):
for j in range(len(self.result.profile_result.list[0])):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for j in range(len(self.result.profile_result.list[0])):
for j in range(1, len(self.result.profile_result.list[0])):

I guess the first can be skipped

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why can the first be skipped? i think the first count can be skipped not j.

Copy link
Member

Choose a reason for hiding this comment

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

Oops, meant count, added a new suggestion.

dilpath and others added 6 commits January 13, 2021 12:36
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
@PaulJonasJost PaulJonasJost merged commit b114683 into develop Jan 15, 2021
@PaulJonasJost PaulJonasJost deleted the features_pypesto_engines branch January 15, 2021 16:07
@yannikschaelte yannikschaelte mentioned this pull request Jan 19, 2021
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.

Parallelize Profile Computation between Parameters
6 participants