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

Refactor and add more benchmarks for montecarlo #2640

Merged
merged 32 commits into from
Jul 12, 2024

Conversation

officialasishkumar
Copy link
Member

@officialasishkumar officialasishkumar commented Jun 2, 2024

📝 Description

Type: 🚀 feature

Refactor montecarlo Bencmarks and adding new benchmarks that cover most of the functions inside montecarlo.

📌 Resources

Examples, notebooks, and links to useful references.

🚦 Testing

How did you test these changes?

  • Testing pipeline
  • Other method (describe)
  • 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.

1 similar comment
@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.

@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.

@tardis-bot
Copy link
Contributor

tardis-bot commented Jun 3, 2024

*beep* *bop*
Hi human,
I ran benchmarks as you asked comparing master (e88a31a) and the latest commit (a245a7d).
Here are the logs produced by ASV.
Results can also be downloaded as artifacts here.
Significantly changed benchmarks:

· No results found

All benchmarks:

· No results found

@officialasishkumar officialasishkumar marked this pull request as draft June 6, 2024 13:34
@AlexHls
Copy link
Member

AlexHls commented Jun 6, 2024

@officialasishkumar Can you please update the PR description, adding some details and a description of your changes?

@officialasishkumar officialasishkumar changed the title [WIP] Refactor benchmarks [WIP] Refactor and add more benchmarks for montecarlo Jun 8, 2024

def time_run_tardis(self):
run_tardis(self.config, log_level="ERROR", show_progress_bars=False)
run_tardis(self.path, log_level="ERROR", show_progress_bars=False)
Copy link
Member

Choose a reason for hiding this comment

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

There should be an empty line at the end of the file.


class BenchmarkTransportMontecarloMainLoop(BenchmarkBase):
"""
class to benchmark montecarlo_main_loop
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being too nit-picky, this can be improved.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure in what sense. Can you please elaborate more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Atharva would like a more descriptive docstring

}
)
def time_kappa_calculation(self, energy):
calculate_opacity.kappa_calculation(energy)
Copy link
Member

Choose a reason for hiding this comment

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

Empty line at end of file

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any vs code extension to automate this task?

assert actual == 0
else:
np.sqrt(r * r - p * p) * formal_integral.C_INV * inv_t
func(r, p, inv_t)

@skip_benchmark
@parameterize({"p": [0, 0.5, 1], "Test data": TESTDATA})
Copy link
Member

Choose a reason for hiding this comment

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

Small question, how does "Test data" know to send its value to test_data?

Copy link
Member Author

Choose a reason for hiding this comment

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

it sends data sequence wise. Parameter name is nothing to do with it.

@andrewfullard andrewfullard mentioned this pull request Jul 3, 2024
6 tasks
@andrewfullard andrewfullard self-requested a review July 9, 2024 13:16
@officialasishkumar officialasishkumar force-pushed the refactor-benchmarks branch 3 times, most recently from b2926ad to c1e4c2f Compare July 9, 2024 14:15
Signed-off-by: Asish Kumar <[email protected]>
Copy link

codecov bot commented Jul 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.67%. Comparing base (0df98ce) to head (ea6175a).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2640      +/-   ##
==========================================
+ Coverage   67.80%   69.67%   +1.87%     
==========================================
  Files         177      181       +4     
  Lines       14534    14468      -66     
==========================================
+ Hits         9855    10081     +226     
+ Misses       4679     4387     -292     

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

@officialasishkumar
Copy link
Member Author

command: asv run refactor-benchmarks^!
Output: https://paste.opensuse.org/pastes/bbf48b94a74c

the benchmarks are ran on all the functions in this PR.

andrewfullard
andrewfullard previously approved these changes Jul 11, 2024
Copy link
Contributor

@andrewfullard andrewfullard left a comment

Choose a reason for hiding this comment

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

Good work. Please address any remaining comments!

Signed-off-by: Asish Kumar <[email protected]>
@officialasishkumar
Copy link
Member Author

Benchmark results for formal_integral functions: https://paste.opensuse.org/pastes/4983e0ca50cb


@property
def simulation_rpacket_tracking_enabled(self):
config_verysimple = self.config_verysimple
Copy link
Contributor

Choose a reason for hiding this comment

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

(Can be future work) Setting up a separate config property or a yml file is cleaner.

@andrewfullard andrewfullard merged commit 17b1da4 into tardis-sn:master Jul 12, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants