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

Restructure code to remove the single_packet_seed field #2036

Merged
merged 3 commits into from
Jun 17, 2022

Conversation

blackbird7112
Copy link
Contributor

📝 Description

Type: 🪲 bugfix

Restructure the code to remove the single_packet_seed field in code, yml and documentation. Fixes #1902 . The single_packet_seed is creating some unexpected bugs. Continuation of #1926

📌 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)

pytest tardis

☑️ 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.

@codecov
Copy link

codecov bot commented May 27, 2022

Codecov Report

Merging #2036 (44a37cb) into master (b394f46) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master    #2036   +/-   ##
=======================================
  Coverage   58.34%   58.34%           
=======================================
  Files          76       76           
  Lines        8599     8593    -6     
=======================================
- Hits         5017     5014    -3     
+ Misses       3582     3579    -3     
Impacted Files Coverage Δ
tardis/montecarlo/base.py 87.60% <ø> (-0.06%) ⬇️
tardis/montecarlo/montecarlo_configuration.py 100.00% <ø> (ø)
tardis/montecarlo/montecarlo_numba/base.py 30.50% <0.00%> (+0.75%) ⬆️
...dis/montecarlo/montecarlo_numba/numba_interface.py 34.64% <ø> (-0.26%) ⬇️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@epassaro
Copy link
Member

What do you think @Rodot-

@@ -5,8 +5,8 @@ To facilitate more in-depth debugging when interfacing with the `montecarlo_numb
module, we provide a set of debugging configurations. PyCharm debugging
configurations, in addition to related scripts and .yml files, are contained in
`tardis.scripts.debug`. Currently, these include the ability to run TARDIS
in asingle-packet mode, with the packet seed identified at debug time.
`tardis_example_single.yml` is the configuration filethat is used to set up the
in a single-packet mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will no longer be true with this change, correct? This particular documentation page is partially deprecated anyway, I think, and should be addressed in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed


with open(yaml_file, "w") as f:
yaml.safe_dump(params, f)
yaml_file = "tardis_example_single.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to other reviewers- this test will need to be addressed in detail in the future.

@Rodot-
Copy link
Contributor

Rodot- commented Jun 6, 2022

Please make sure the updated files pass the black check and add yourself to the mailmap. Other than that, this is looking good as it's passing the tests.

@blackbird7112
Copy link
Contributor Author

I have made all updates as per the discussions. Please have another look into the PR

@andrewfullard andrewfullard merged commit bb12b44 into tardis-sn:master Jun 17, 2022
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.

Remove single_packet_seed from config and across the code
5 participants