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

Update of the mesa_r15140 interface #1099

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

lucasciarini
Copy link

Update of the mesa_r15140 interface, adding getters required to make it compatible with TRES:

  • get_core_radius
  • get_convective_envelope_mass
  • get_convective_envelope_radius
  • get_wind_mass_loss_rate
  • get_apsidal_motion_constant
  • get_gyration_radius

Update of the evolve_until subroutine in mesa_interface.f90 solving timestep issues occuring in the previous version when the subroutine was called multiple times in a row.

The modified script files are interface.py, interface.f90 and mesa_interface.f90, all in src/amuse/community/mesa_r15140.

Tests scripts are provided in src/amuse/community/mesa_r15140/Tests.

Update of the mesa_r15140 interface, adding getters required to make it compatible with TRES:
- get_core_radius<
- get_convective_envelope_mass
- get_convective_envelope_radius
- get_wind_mass_loss_rate
- get_apsidal_motion_constant
- get_gyration_radius

Update of the evolve_until subroutine in mesa_interface.f90 solving timestep issues occuring in the previous version when the subroutine was called multiple times in a row.
Two test scripts are provided. There are located in src/amuse/community/mesa_r15140/Tests

test_new_getters.py tests the functionality of the getters added to the interace.

test_updated_evolve_until.py tests the functionality of the updated evolve_until function in mesa_interface.f90.
@rieder rieder self-requested a review December 18, 2024 13:30
@LourensVeen
Copy link
Collaborator

Hi Luca,

Thanks for this PR! I'll let Steven judge the astrophysics, but code quality looks fine to me overall. I do have two more software-technical points.

First, you added tests, which is great! However, they plot their results, which means that a human would have to look at those plots to see if they make sense. We'd really like to be able to run the tests automatically (there are a bunch of tests in src/amuse/tests actually that are set up for this), for which we need some assert statements in the test that check whether the result is as expected, and which crash otherwise.

Would it be possible to add those?

I'm currently nearing the end of a big project to replace the AMUSE build system, and that also reorganises those tests in src/amuse/tests, so it's fine to have them as separate scripts in Tests/. I'll merge them in with the rest when that new build system lands so that they can be run automatically with everything else, ensuring that your additions won't be broken by some future change.

Second, the fix to the loop causes some repetition, and I think this part could be written more compactly, but I need to have a more careful look at a more reasonable hour. I'll post if I find a better solution.

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