rice-solar-physics / ebtelplusplus

An implementation of the enthalpy-based thermal evolution of loops (EBTEL) model implemented in C++ and wrapped in Python
https://ebtelplusplus.readthedocs.io
GNU General Public License v3.0
11 stars 5 forks source link

Add functionality for time-variable abundances and radiative losses #87

Closed jwreep closed 4 months ago

jwreep commented 10 months ago

New functionality introduced to allow the abundance factor to vary with time, and thus the radiative losses to vary with time.

jwreep commented 10 months ago

@wtbarnes, if we're changing abundances, does this function need to be updated?

https://github.com/rice-solar-physics/ebtelPlusPlus/blob/4918ca9475fb5503d465a2e6a19fcd1c99fdb966/source/loop.cpp#L416-L421

jwreep commented 6 months ago

I slightly reworked the input functionality so that time-fixed coronal and time-fixed photospheric abundances are now valid options.

wtbarnes commented 6 months ago

If you rebase or merge in main, you should pick up the C++-14 changes to SConstruct.

jwreep commented 5 months ago

Ready for review!

Since this is a breaking change, I think we should review carefully. Would any additional tests be useful?

wtbarnes commented 4 months ago

@wtbarnes, if we're changing abundances, does this function need to be updated?

https://github.com/rice-solar-physics/ebtelPlusPlus/blob/4918ca9475fb5503d465a2e6a19fcd1c99fdb966/source/loop.cpp#L416-L421

Honestly, I'm not sure. I need to go back and remind myself what the logic of that function even is. I think it mainly has to do with the H/He ratio though. How impacted is that by the transition from coronal to photospheric?

I'm also a little worried about the name of this function given that it potentially is unrelated to the abundance variations in the radiative losses. I'm fine with changing it to something else.

jwreep commented 4 months ago

Honestly, I'm not sure. I need to go back and remind myself what the logic of that function even is. I think it mainly has to do with the H/He ratio though. How impacted is that by the transition from coronal to photospheric?

I'm also a little worried about the name of this function given that it potentially is unrelated to the abundance variations in the radiative losses. I'm fine with changing it to something else.

It looks like that's what this function does. Helium does not change its abundance in what we've programmed here, although I've been tinkering with the idea of generalizing this for other stellar classes (some stars have an inverse FIP bias). In that case, it's possible that helium might change with time.

jwreep commented 4 months ago

@wtbarnes, I'm having trouble adding tests, and not sure what the issue is. I keep getting something similar to this when running pytest, for even very simple tests:

========================================================= short test summary info =========================================================
ERROR tests/test_solver.py::test_coronal_output - FileNotFoundError: /var/folders/5r/lmtf450n5fn0xrgg3t8689m40000gn/T/tmpesgpj0th/ebtelplusplus.tmp not found.

The code I've added to tests/test_solver.py:

@pytest.fixture(scope='module')
def coronal_results(base_config):
    config = base_config.copy()
    config['radiation'] = "coronal"
    return run_ebtelplusplus(config)

def test_coronal_output(coronal_results):
    time = coronal_results['time'].to_value('s')
    assert np.isclose(time[0], 0.0)

Any idea here?

Other than the tests, I think I've addressed everything else.

wtbarnes commented 4 months ago

Ok, I've added a really basic test for each of the new radiative loss options to essentially just make sure that it can be run with that option without erroring. I'm not sure why you were seeing the error you were seeing with pytest.

Because I've pushed to your branch, make sure to pull down the latest changes before making any more commits to avoid any "fun" conflicts!

jwreep commented 4 months ago

Thanks for the help with that! I have no idea the cause of the error. Pytest runs A-okay without that added code, though.

wtbarnes commented 4 months ago

Thanks for all of your efforts per usual @jwreep!