mesa icon indicating copy to clipboard operation
mesa copied to clipboard

Pgstar inlist arrays

Open matthiasfabry opened this issue 1 year ago • 9 comments

Large refactor of the pgstar infrastructure (put as draft for now).

The main change is to have the windows with multiple entries (Text_Summary(1-9), History_track(1-9), History_panels(1-9), Color_magnitude(1-9), Grid(1-9)). be (mostly) handled in arrays, in the spirit of #368. This reduces the required defaults by several thousands of lines, and cleans up the *.f90 files of the affected windows, aiding code maintenance.

Only thing that isn't yet possible is having procedure pointers in an array, which means that the decorator pointers are unaffected. As a matter of fact, I would like to push for dropping support for these functions; if users want custom plotting/annotations, I feel like they are much better off using their own library. This would further simplify our pgstar implementation.

I need help again however from the sed wizards for devising clever substitution scripts, where not only Grid2_* is changed to Grid_*(2), but also Grid2_*(1) to Grid_*(2, 1), etc.

matthiasfabry avatar Jun 13 '23 13:06 matthiasfabry

is

setenv MESA_FORCE_PGSTAR_FLAG true

sufficient for these changes get tested within the test suite?

fxt44 avatar Jun 13 '23 14:06 fxt44

Promoted this out of draft status. What remains to do is check whether all seds have been done correctly. The test suite seems to pick up some of them

matthiasfabry avatar May 29 '24 13:05 matthiasfabry

Do you have a list of the regular expressions/find+replace commands you used to make the changes? I've set up a little sandbox to start experimenting but figured I'd just be reinventing what you've already done if you used regexes in the first place.

warrickball avatar May 31 '24 20:05 warrickball

I have not, I used the find and replace feature of my IDE to do most of the renaming. @mjoyceGR sent me a good starting point to also get into seding:

s/Grid\(.\)_*(/Grid_*(\1, /g
s/Grid\([^_]\)_*/Grid_*(\1)/g

If this covers everything, then what remains is to repeat this for Profile_Panels, History_Panels, History_Track, Text_Summary, Color_Magnitude, including entries in pgbinary.

matthiasfabry avatar Jun 03 '24 08:06 matthiasfabry

That's helpful, thanks! I've just been experimenting with that in my sandbox and currently have

sed -E \
    -e 's/(History_Track|Grid|Profile_Panels|History_Panels|Text_Summary)([1-9])([_a-zA-Z]*) = /\1\3\(\2\) = /' \
    -e 's/(History_Track|Grid|Profile_Panels|History_Panels|Text_Summary)([1-9])([_a-zA-Z]*)\(/\1\3\(\2\,/' \
    inlist_original > inlist_modified

where inlist_original is the inlist that you want to change. I haven't tested this but it seems to catch basically everything. I generated test data by dumping everything in */test_suite/*/inlist_pgstar to a couple of files and, though there are a bunch of changes on main, I didn't see anything obvious left.

I'll try to construct my test data a bit more carefully and iterate again in the next few days but I think this covers most cases.

warrickball avatar Jun 09 '24 20:06 warrickball

What is the status of this PR? Is it nearly ready to go?

I can help fix conflicts with main (a lot of it is due to removing unused variables in star, or updates and formatting to inlists) and check the seds

pmocz avatar Aug 23 '24 23:08 pmocz

I think it's worth fixing the conflicts with main because this is a change we should implement at some point.

My previous comment is as far as I got with sed commands to fix existing inlists. If a sure-fire way of updating inlists is the blocker, I can prioritise this (though that doesn't mean "do it soon").

It's probably worth a look now that we've just made a release. We should have at least a few months to bash our own heads against this before it becomes part of another release.

warrickball avatar Aug 24 '24 19:08 warrickball

I think it's worth fixing the conflicts with main because this is a change we should implement at some point.

My previous comment is as far as I got with sed commands to fix existing inlists. If a sure-fire way of updating inlists is the blocker, I can prioritise this (though that doesn't mean "do it soon").

It's probably worth a look now that we've just made a release. We should have at least a few months to bash our own heads against this before it becomes part of another release.

I fixed conflicts with main. And applied seds to inlists I could find with grepping. Perhaps we could merge into main after we take another look for outstanding inlists and add an sed script to help users change their inlists, + online documentation, so that it can start being tested before next release?

pmocz avatar Aug 25 '24 15:08 pmocz