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

Rename MontecarloRunner to MontecarloTransport #2317

Conversation

xansh
Copy link
Member

@xansh xansh commented Jun 9, 2023

📝 Description

Type: 🎢 infrastructure

This PR renames MontecarloRunner to MontecarloTransport (and all such mentions of runner, montecarlo_runner, etc).
It also updates the docs and .hdf files accordingly.

Note: This PR also needs the reference data to be updated, for which a PR is logged here.

Sibling Request

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

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@xansh
Copy link
Member Author

xansh commented Jun 9, 2023

@wkerzendorf @andrewfullard Please review this.

@xansh
Copy link
Member Author

xansh commented Jun 9, 2023

Note that the tests are failing because they're testing the code changes against the old tardis-refdata. Ideally they should test the changes after this PR is merged.

@@ -1,13 +1,15 @@
{
"cells": [
{
"attachments": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know what this change is?

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewfullard I'm not sure, Jupyter seems to have done it automatically on file save. (I just changed the occurrences of "runner" to "transport", didn't even run any cell.)

Copy link
Member

Choose a reason for hiding this comment

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

it's a new version of Jupyter notebook likely.

@codecov
Copy link

codecov bot commented Jun 10, 2023

Codecov Report

Merging #2317 (96a9820) into master (9dd96fa) will not change coverage.
The diff coverage is 76.02%.

❗ Current head 96a9820 differs from pull request most recent head 2ebf336. Consider uploading reports for the commit 2ebf336 to get more accurate results

@@           Coverage Diff           @@
##           master    #2317   +/-   ##
=======================================
  Coverage   71.96%   71.96%           
=======================================
  Files         137      137           
  Lines       12514    12514           
=======================================
  Hits         9006     9006           
  Misses       3508     3508           
Impacted Files Coverage Δ
tardis/analysis.py 32.40% <ø> (ø)
tardis/io/tests/test_HDFWriter.py 100.00% <ø> (ø)
tardis/io/util.py 77.05% <ø> (ø)
tardis/montecarlo/tests/test_montecarlo.py 30.15% <ø> (ø)
tardis/tests/integration_tests/test_integration.py 29.26% <ø> (ø)
tardis/util/base.py 74.20% <ø> (ø)
tardis/visualization/widgets/line_info.py 95.94% <ø> (ø)
tardis/simulation/tests/test_simulation.py 91.02% <33.33%> (ø)
tardis/io/model_reader.py 67.49% <36.20%> (ø)
...ontecarlo_numba/tests/test_cuda_formal_integral.py 43.35% <50.00%> (ø)
... and 12 more

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

@andrewfullard andrewfullard enabled auto-merge (squash) June 12, 2023 15:00
@andrewfullard andrewfullard merged commit 846ef7c into tardis-sn:master Jun 13, 2023
@xansh xansh self-assigned this Jun 13, 2023
@xansh xansh deleted the rename-MontecarloRunner-to-MontecarloTransport branch June 22, 2023 19:59
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.

3 participants