search

MESAHub / mesa

Modules for Experiments in Stellar Astrophysics
http://mesastar.org
GNU Lesser General Public License v2.1
138 stars 38 forks source link

num: make test src f2008 standard compliant #696

Closed VincentVanlaer closed 1 month ago

warrickball commented 2 months ago

Nice work Vincent! The changes look good to me (given a ~2 minute glance). I'm not sure if it's clear to everyone but I think the process with external PRs is still to merge into a new branch in the MESAHub repo (e.g. MESAHub:num-test-stanadard) so we can run the integrations tests, then we merge that into MESAHub:main.

Let's make all of MESA f2008 standard compliant. I had recently added some changes like this one myself a couple weeks ago

:100:

I realise it's a bit of a digression but I thought I'd mention that I tried to take on and basically abandoned an effort to modernise the OP kap routines in #527. You can see my rationale there but it would help barrel down on why we still occasionally get bit-for-bit differences in the radiative_levitation test case.

VincentVanlaer commented 2 months ago

Let's make all of MESA f2008 standard compliant. I had recently added some changes like this one myself a couple weeks ago

Currently that's the only thing the new build system supports. That is also why I made this PR. I noticed that I missed that some of the main source files of the num module fail, so I'll add those to this PR as well.

I'm not sure if it's clear to everyone but I think the process with external PRs is still to merge into a new branch in the MESAHub repo (e.g. MESAHub:num-test-stanadard) so we can run the integrations tests, then we merge that into MESAHub:main.

I don't think I can do that since the branch doesn't exist (or at least not after the PR is created).

warrickball commented 2 months ago

I'm not sure if it's clear to everyone but I think the process with external PRs is still to merge into a new branch in the MESAHub repo (e.g. MESAHub:num-test-stanadard) so we can run the integrations tests, then we merge that into MESAHub:main.

I don't think I can do that since the branch doesn't exist (or at least not after the PR is created).

Yeah, as you say, I'm pretty sure someone with write access to MESAHub/mesa repo needs to do that. I'll ping @evbauer as someone who I think has handled that before, though I'm happy to do it this time.

warrickball commented 2 months ago

I just built VincentVanlaer:num-test-standard using MESA SDK 23.7.3 and got many warnings, which I'll summarise as

You can have a look at the output in one of the CI actions (e.g. the MESA SDK 23.7.3 build).

These are all in num itself, which I guess you haven't modernised but have somehow changed the compiler flags to no longer treat it as legacy code. Could you fix the relevant flag?

VincentVanlaer commented 2 months ago

I made the minimal changes necessary to make the files compile under -std=f2008, ignoring warnings since they just indicate what could be improved, but is still standard compliant. It's just a matter of dropping the final commit to get rid of them, but I would consider them a good reminder to fix these issues at some point in the future.