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

Add a ModelState class #1847

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Conversation

atharva-2001
Copy link
Member

Description

Motivation and context

How has this been tested?

  • Testing pipeline.
  • Other.

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 Dec 23, 2021

Codecov Report

Merging #1847 (5ef235b) into master (c34beca) will increase coverage by 4.17%.
The diff coverage is n/a.

❗ Current head 5ef235b differs from pull request most recent head 1c4e1fe. Consider uploading reports for the commit 1c4e1fe to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   58.04%   62.21%   +4.17%     
==========================================
  Files          66       66              
  Lines        6747     6826      +79     
==========================================
+ Hits         3916     4247     +331     
+ Misses       2831     2579     -252     
Impacted Files Coverage Δ
...dis/montecarlo/montecarlo_numba/numba_interface.py 35.46% <0.00%> (-10.69%) ⬇️
tardis/tardis/analysis.py 30.16% <0.00%> (ø)
...rdis/tardis/montecarlo/montecarlo_configuration.py 100.00% <0.00%> (ø)
tardis/tardis/montecarlo/base.py 88.94% <0.00%> (+0.17%) ⬆️
tardis/tardis/simulation/base.py 83.79% <0.00%> (+0.46%) ⬆️
tardis/tardis/model/base.py 88.96% <0.00%> (+0.66%) ⬆️
.../montecarlo/montecarlo_numba/single_packet_loop.py 29.78% <0.00%> (+2.66%) ⬆️
tardis/tardis/montecarlo/montecarlo_numba/base.py 32.14% <0.00%> (+5.92%) ⬆️
tardis/tardis/visualization/plot_util.py 100.00% <0.00%> (+6.25%) ⬆️
tardis/tardis/base.py 75.00% <0.00%> (+17.85%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c34beca...1c4e1fe. Read the comment docs.

@@ -23,6 +23,11 @@
logger = logging.getLogger(__name__)


class ModelState():
def __init__(self):
Copy link
Member

Choose a reason for hiding this comment

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

you want to initialize it with the variables as discussed

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 2e58f01 a good way to do this? Because properties are very related to each other, I thought it might be good to move those calculations to model_state class.

tardis/model/base.py Outdated Show resolved Hide resolved
tardis/model/base.py Outdated Show resolved Hide resolved
tardis/model/base.py Outdated Show resolved Hide resolved
tardis/model/base.py Outdated Show resolved Hide resolved
tardis/model/base.py Outdated Show resolved Hide resolved
@andrewfullard andrewfullard self-requested a review January 5, 2022 16:53
@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.

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.

I think the class looks good, though it should in the future probably act as a base class for the Radial1DModel. Currently, the ModelState assumes 1D as well. Unclear if we want to generalise more right now.

@wkerzendorf
Copy link
Member

@atharva-2001 can you check the docststrings - but then it is good to merge.

@atharva-2001 atharva-2001 self-assigned this Jan 19, 2022
@atharva-2001 atharva-2001 force-pushed the model_state branch 5 times, most recently from 0afe798 to e32ee17 Compare January 21, 2022 10:10
@wkerzendorf wkerzendorf merged commit d80d205 into tardis-sn:master Jan 24, 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.

4 participants