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

[GSOC] Benchmarking and Performance Improvement: objective 1 #2547

Conversation

haleelsada
Copy link

@haleelsada haleelsada commented Mar 12, 2024

📝 Description

Type: :🌞 GSoC | 🚀 feature

PR on first objective of idea TARDIS Benchmarking and Performance Improvement GSOC'24.
Added 2 benchmarks for SDEC plotter using Matplotlib and Plotly. Observed Plotly is much faster than Matplotlib.
Implemented by first running run_tardis then benchmarking the SDEC plotting using libraries Plotly(generate_plot_ply) and Matplotlib(generate_plot_mpl) on the output of run_tardis.

plot_mpl2

plot_ply2

📌 Resources

Tardis Ideas

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)(Ran the program and observed results)
  • My changes can't be tested (explain why)

☑️ Checklist

  • I requested two reviewers for this pull request
  • I updated the documentation according to my changes
  • I built the documentation by applying the build_docs label

Note: If you are not allowed to perform any of these actions, ping (@) a contributor.

@tardis-bot
Copy link
Contributor

*beep* *bop*

Hi, human.

I'm the @tardis-bot and couldn't find your records in my database. I think we don't know each other, or you changed your credentials recently.

Please add your name and email to .mailmap in your current branch and push the changes to this pull request.

In case you need to map an existing alias, follow this example.

@andrewfullard
Copy link
Contributor

Please provide graphical output and how you achieved the output

@haleelsada
Copy link
Author

haleelsada commented Mar 13, 2024

Please provide graphical output and how you achieved the output

Done
I accidentally closed this pull request. I've reopened it now. Sorry for the inconvenience

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.17%. Comparing base (20b2a64) to head (f31b95a).
Report is 10 commits behind head on master.

❗ Current head f31b95a differs from pull request most recent head 8f42743. Consider uploading reports for the commit 8f42743 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2547   +/-   ##
=======================================
  Coverage   68.17%   68.17%           
=======================================
  Files         166      166           
  Lines       14135    14135           
=======================================
  Hits         9636     9636           
  Misses       4499     4499           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrewfullard
Copy link
Contributor

Thanks for the images! Please refer to this pull request in your application.

@andrewfullard
Copy link
Contributor

What command did you run to produce the asv web output? How long did it take to run the benchmark? Why did you choose the number of commits that are shown in the plot?

@andrewfullard
Copy link
Contributor

Are you applying for the shorter or larger project? If the larger, please produce the memory profile as requested. That can be a separate PR.

@haleelsada
Copy link
Author

haleelsada commented Mar 14, 2024

What command did you run to produce the asv web output?

first collected hash of last ten commits, then implemented this commands
asv run HASHFILE:tag_commits.txt
asv publish
asv preview

How long did it take to run the benchmark?

10-15 mins

Why did you choose the number of commits that are shown in the plot?

I have benchmarked last ten commits. since they depends on the speed of libraries Matplotlib and Plotly, I concentrated on comparing both the methods.

Are you applying for the shorter or larger project? If the larger, please produce the memory profile as requested. That can be a separate PR.

I am mainly interested in the topic benchmarking. And Yes, I will produce another PR on memory profiler, thank you!

params = ['branch85_w7', 'uniform','power_law','exponential']
param_names = ['Density']

def setup(self, d):
Copy link
Contributor

Choose a reason for hiding this comment

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

d is not a good variable name, please use something more descriptive

Copy link
Author

Choose a reason for hiding this comment

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

I have changed it.

config.model.structure.velocity.stop = 2000 * u.km/u.s
config.model.structure.velocity.num = 20
if d == 'uniform':
config.model.structure.density.time_0 = 1 * u.day
Copy link
Contributor

Choose a reason for hiding this comment

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

If many of these settings are the same, they don't need to be in the if statements

Copy link
Author

Choose a reason for hiding this comment

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

I have added the common settings outside the if clause and anything that differ from default inside the if clause. I thought it would be better for readability.

Parameter is changed from d to density for better readability in benchmark_run_tardis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants