made the timo_0 in decay framework required

[Soumya624]

📝 Description

Type: 🪲 bugfix | 🚀 feature

Changed the current init method in line 11 of "\tardis\io\decay.py" file, to resolve the issue. Here is a snippet of the modified version of the init method that includes a check to ensure that the time_0 parameter is specified by the user:

This modified version first checks if the time_0 parameter is included in the kwargs dictionary. If it is not, it raises a ValueError to inform the user that the time_0 parameter must be specified. If the time_0 parameter is present, it is extracted from the kwargs dictionary and assigned to the time_0 variable. The time_0 key is then removed from the kwargs dictionary before calling the parent class's init method. Finally, the value of time_0 is assigned to the time_0 attribute of the IsotopeAbundance instance.

Also, link issues affected by this pull request by using the keywords: close, closes, closed, fix, fixes, fixed, resolve, resolves or resolved.

📌 Resources

Examples, notebooks and links to useful references.

🚦 Testing

How did you test these changes?

• Testing Locally

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

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

[Rodot-]

This seems to break the tests.

[sonachitchyan]

This seems to break the tests.

The tests probably assume that the "default value" is set to 0, as it was in previously. @Soumya624 , can you look into that?

[Soumya624]

Yeah sure!

[andrewfullard]

@Soumya624 do you plan to address the currently failing checks soon?

[AyushiDaksh]

@Soumya624 Are you still working on resolving this? If not, please let me know if I can take it forward from here.

[Soumya624]

The tests probably assume that the "default value" is set to 0, as it was in previously

@andrewfullard I was going through the entire repository and i found that any test cases that create an instance of the IsotopeAbundance class without explicitly specifying the time_0 parameter are likely to fail.

So, I searched the test cases and found that if we can modify 'tardis/io/tests/test_decay.py' and 'tardis/model/tests/test_base.py', then we can omit these.

To modify the simple_abundance_model() fixture in tardis/io/tests/test_decay.py to avoid the failure due to the time_0 parameter not being specified, we can add an explicit time_0 parameter with a default value of 0 to the function definition and pass this default value to the constructor of IsotopeAbundances.

@pytest.fixture
def simple_abundance_model(time_0=0):
    index = pd.MultiIndex.from_tuples(
        [(28, 56)], names=["atomic_number", "mass_number"]
    )
    return IsotopeAbundances([[1.0, 1.0]], index=index, time_0=time_0)

And to modify the simple_isotope_abundance() function in tardis/model/tests/test_base.py, we can add a similar explicit time_0 parameter with a default value of 0 to the function definition and pass this default value to the constructor of IsotopeAbundances.

def simple_isotope_abundance(time_0=0):
    index = pd.MultiIndex.from_tuples(
        [(6, 14), (12, 28)], names=["atomic_number", "mass_number"]
    )
    abundance = [[0.2] * 20] * 2
    return IsotopeAbundances(abundance, index=index, time_0=time_0)

Please let me know if this approach is correct. I will made the PR!

[Soumya624]

@Soumya624 Are you still working on resolving this? If not, please let me know if I can take it forward from here.

@AyushiDaksh Yeah i'm about to make a PR. With the necessary changes in the test cases!

[andrewfullard]

Sorry for the late response. I think your solution is sensible, please commit it to this PR.

[AyushiDaksh]

@Soumya624 Are you still working on this?

@andrewfullard If @Soumya624 is not actively working on this, then I'll take this up. In that case, would I have to make a new PR or append my changes to this PR?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[Soumya624]

Hi @andrewfullard,
Please have a look at this now. Updated the changes!

Thanks

[Soumya624]

@Soumya624 Are you still working on this?
Yeah. think rn it's passing the tests. Thanks for reminding!

[andrewfullard]

Please resolve the merge conflict

[Soumya624]

Hi @andrewfullard ,

Thanks for your review regarding the variable name. Please have a look at this now!

[andrewfullard]

@wkerzendorf do we want to require "time_0" now that we are changing how the model is set up?

[andrewfullard]

Closing this in favour of later configuration restructure