Changes for upper and lower errors when determining fundamental parameters.

[SherelynA]

Addresses issue #88 by calculating upper and lower error for evolutionary model derived parameters using the already integrated Monte-Carlo approach along with using a 68% central confidence interval. The method used is described in Suárez & Metchev (2022).

I was unable to run tests for the changes nonetheless, in the future tests should be written for these additions.

[kelle]

Now that tests are re-running, let's put this on hold until we can more of the old tests to pass.

[kelle]

It looks like the tests need to be modified as a result of these changes.

[kelle]

It would be ideal to get this merged and part of the next release.

[SherelynA]

@kelle the tests seem to work !

[kelle]

This looks great! Minor comments to delete some comment blocks.

Only thing it needs is a couple new tests. We can discuss at our meeting later today.

[kelle]

@hover2pi , this is very close to being merged. Please take a look! And let us know if you have any ideas for tests for this new functionality.

[hover2pi]

Looks good to me. Thanks for all the awesome improvements @SherelynA !

[SherelynA]

@hover2pi and @kelle, I have this index.p file that shows up in my changes though I don't recall changing it, do you guys have an idea about what it may be and if I should be committing it too?

[kelle]

I had the same question! It's a cache file and doesn't need to be added to the repo.

I think we should add to the gitignore. I'll open a new issue. #117

[hover2pi]

Pretty sure that's an index for the model atmosphere files to greatly speed up the model fitting. The code looks for it when fitting models and generates it automatically if it's absent so you could add it to the .gitignore. When a user wants to fit any model grids, they'll just have to wait ~2 minutes the first time they try but it prints a warning if I remember correctly.

[SherelynA]

@kelle and @hover2pi right now I am writing some additional tests for different scenarios of uncertainties for the xval and yparams, and when we provide no xval or yparam uncertainty, the mass is way off that what we calculated, so like for example the other cases gives something like 0.072 Sol masses and the last scenario gives 0.020 something,

so I am a bit confused on how to write a test that checks for all the values without the last one failing. I had assert (np.isclose(result[0].value, 0.074, atol=0.005)) but you can see how this would fail since we are using python mark paramterize and now it goest through a loop and the last one is not close at all.

[kelle]

please commit and push the code which is failing.. Nevermind, see below suggestion for to include different expected results with pytest.mark.parametrize

[SherelynA]

@kelle and @hover2pi, logg doesn't have units since it is a log but couldn't we technically provide it the astropy unit of u.dex?

[SherelynA]

@hover2pi I see in an old test you wrote, in test_isochrone.py, you tested a scenario where the lbol and age don't have uncertainties, but I guess I wanted to know in what scenario you think we could get something like this?

[hover2pi]

@SherelynA I suspect that test was just to check the no-uncertainty handling for arbitrary variables, not a specific use case. Feel free to change it to something more probative!

[hover2pi]

I also remember trying to use dex units and it was problematic for some reason that I can't recall. You could try to put that in but it might me a minefield and I'm not sure we gain much since the functions already check for units and handle them accordingly. Admittedly the hasattr('unit') check isn't ideal but I would recommend only supporting dex units if it actually fixes something that is broken.

[SherelynA]

I also remember trying to use dex units and it was problematic for some reason that I can't recall. You could try to put that in but it might me a minefield and I'm not sure we gain much since the functions already check for units and handle them accordingly. Admittedly the hasattr('unit') check isn't ideal but I would recommend only supporting dex units if it actually fixes something that is broken.

@hover2pi Yeah, I agree that I don't think we gain much by trying to add it. The only reason I asked was because during checking tests I realized that all the results had units except logg so I had to change accordingly. However, since you mentioned it being problematic I think it is best to just have the the condition in the tests and not try to add units.

[hover2pi]

Fantastic. Thanks @SherelynA !

[kelle]

Great work!!