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 #1926

Closed

Conversation

blackbird7112
Copy link
Contributor

Description

Restructure the code to remove the single_packet_seed field in code, yml and documentation.

Motivation and context

Fixes #1902 . The single_packet_seed is creating some unexpected bugs

How has this been tested?

  • Testing pipeline.
  • Other.
    pytest tardis

Examples

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@codecov
Copy link

codecov bot commented Mar 13, 2022

Codecov Report

Merging #1926 (c19b0ec) into master (b5ab813) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head c19b0ec differs from pull request most recent head 955a3dc. Consider uploading reports for the commit 955a3dc to get more accurate results

@@           Coverage Diff           @@
##           master    #1926   +/-   ##
=======================================
  Coverage   58.74%   58.75%           
=======================================
  Files          70       70           
  Lines        7847     7841    -6     
=======================================
- Hits         4610     4607    -3     
+ Misses       3237     3234    -3     
Impacted Files Coverage Δ
...dis/montecarlo/montecarlo_numba/numba_interface.py 35.95% <0.00%> (-0.27%) ⬇️
tardis/tardis/montecarlo/base.py 86.44% <0.00%> (-0.07%) ⬇️
...rdis/tardis/montecarlo/montecarlo_configuration.py 100.00% <0.00%> (ø)
tardis/tardis/montecarlo/montecarlo_numba/base.py 32.07% <0.00%> (+0.88%) ⬆️

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

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@@ -5,7 +5,6 @@ definitions:
title: 'Damped Convergence Strategy'
type: object
additionalProperties: false
properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a relevant bugfix to this 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.

Not exactly. But while working on this PR I saw this small error and fixed it. Do you think I should put this in a separate commit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. We prefer to limit PRs to fix one item at a time whenever possible.

import os
import numba
import sys
import yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried running this script? Does it still work, and did it work before your changes?

Copy link
Contributor Author

@blackbird7112 blackbird7112 Mar 16, 2022

Choose a reason for hiding this comment

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

I have tested the new script and it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before the changes where made, the python script needs some argument for the single_packet_seed in the yaml file. But there is no documentation on the valid values that can passed to the python script. Also the line SEED = eval(sys.argv[1].split("=")[1])[0] is quite confusing. Could you provide a valid argument that can be passed to the script?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the argument is intended to be something like seed=1234 based on the syntax, but if the script runs then it's okay.

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

@blackbird7112
Copy link
Contributor Author

@andrewfullard I have removed the irrelevant bugfix and updated the PR. Please have a look at the changes

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
4 participants