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 functionality for time-variable abundances and radiative losses #87

Merged
merged 25 commits into from
Jul 5, 2024

Conversation

jwreep
Copy link
Member

@jwreep jwreep commented Jan 10, 2024

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

@jwreep
Copy link
Member Author

jwreep commented Jan 11, 2024

@jwreep
Copy link
Member Author

jwreep commented May 11, 2024

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

@wtbarnes
Copy link
Member

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

@jwreep jwreep marked this pull request as ready for review June 3, 2024 08:40
@jwreep
Copy link
Member Author

jwreep commented Jun 3, 2024

Ready for review!

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

@wtbarnes
Copy link
Member

wtbarnes commented Jul 2, 2024

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

Copy link
Member

@wtbarnes wtbarnes left a comment

Choose a reason for hiding this comment

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

Sorry this has taken so long to get to. Overall, I'm fine with the user-facing part of this functionality. However, I'm mainly concerned with hardcoding the sizes of the arrays and the corresponding abundance factors and densities these correspond to. I've left several comments to this effect.

I also think we should add a few tests that use these three new inputs just to ensure that they keep working. I'm happy to do that.

source/loop.cpp Outdated Show resolved Hide resolved
source/helper.h Outdated Show resolved Hide resolved
source/helper.h Outdated Show resolved Hide resolved
source/loop.cpp Show resolved Hide resolved
source/loop.cpp Outdated Show resolved Hide resolved
source/loop.cpp Outdated Show resolved Hide resolved
source/loop.cpp Outdated Show resolved Hide resolved
source/loop.cpp Outdated Show resolved Hide resolved
@jwreep
Copy link
Member Author

jwreep commented Jul 3, 2024

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
Copy link
Member Author

jwreep commented Jul 4, 2024

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

source/util/misc.cpp Outdated Show resolved Hide resolved
@wtbarnes
Copy link
Member

wtbarnes commented Jul 4, 2024

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
Copy link
Member Author

jwreep commented Jul 4, 2024

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 wtbarnes merged commit 39d7c82 into rice-solar-physics:main Jul 5, 2024
2 checks passed
@wtbarnes
Copy link
Member

wtbarnes commented Jul 5, 2024

Thanks for all of your efforts per usual @jwreep!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants