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

Make nthreads a property of MontecarloRunner instead of Simulation class #2311

Merged
merged 7 commits into from Jun 15, 2023

Conversation

xansh
Copy link
Member

@xansh xansh commented Jun 8, 2023

📝 Description

Type:🎢 restructure

Made nthreads a property of MontecarloRunner instead of passing it from Simulation

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

@xansh xansh marked this pull request as draft June 8, 2023 13:33
@xansh xansh marked this pull request as ready for review June 9, 2023 08:58
@xansh xansh changed the title Made nthreads a property of MontecarloTransport Make nthreads a property of MontecarloRunner instead of Simulation class Jun 9, 2023
@xansh
Copy link
Member Author

xansh commented Jun 9, 2023

Hi @wkerzendorf @andrewfullard

Can you please review this?

Thanks!

@codecov
Copy link

codecov bot commented Jun 9, 2023

Codecov Report

Merging #2311 (b275063) into master (846ef7c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b275063 differs from pull request most recent head 6863d4f. Consider uploading reports for the commit 6863d4f to get more accurate results

@@           Coverage Diff           @@
##           master    #2311   +/-   ##
=======================================
  Coverage   71.96%   71.97%           
=======================================
  Files         137      137           
  Lines       12514    12516    +2     
=======================================
+ Hits         9006     9008    +2     
  Misses       3508     3508           
Impacted Files Coverage Δ
tardis/io/model_reader.py 67.49% <ø> (ø)
tardis/simulation/base.py 83.92% <ø> (-0.08%) ⬇️
tardis/io/tests/test_model_reader.py 97.90% <100.00%> (+0.01%) ⬆️
tardis/montecarlo/base.py 87.08% <100.00%> (+0.10%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

wkerzendorf
wkerzendorf previously approved these changes Jun 9, 2023
Copy link
Member

@wkerzendorf wkerzendorf 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 except potentially nthreads=1

tardis/montecarlo/base.py Outdated Show resolved Hide resolved
@andrewfullard andrewfullard self-requested a review June 9, 2023 12:45
@wkerzendorf wkerzendorf enabled auto-merge (squash) June 9, 2023 17:45
auto-merge was automatically disabled June 11, 2023 19:26

Head branch was pushed to by a user without write access

@xansh xansh self-assigned this Jun 13, 2023
@wkerzendorf wkerzendorf merged commit b79a6f8 into tardis-sn:master Jun 15, 2023
7 checks passed
@xansh xansh deleted the MonteCarlo_nthreads branch June 22, 2023 19:58
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.

None yet

4 participants