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

Refactor read_extra_..._inlist into arrays #368

Closed
warrickball opened this issue Feb 7, 2022 · 3 comments
Closed

Refactor read_extra_..._inlist into arrays #368

warrickball opened this issue Feb 7, 2022 · 3 comments
Labels
good first issue Good for newcomers

Comments

@warrickball
Copy link
Contributor

Since time immemorial (before r4366), various MESA inlists have had the option to read other inlists recursively, with options like

    read_extra_star_job_inlist1 = .false.
    extra_star_job_inlist1_name = 'undefined'
    read_extra_star_job_inlist2 = .false.
    extra_star_job_inlist2_name = 'undefined'
    read_extra_star_job_inlist3 = .false.
    extra_star_job_inlist3_name = 'undefined'
    read_extra_star_job_inlist4 = .false.
    extra_star_job_inlist4_name = 'undefined'
    read_extra_star_job_inlist5 = .false.
    extra_star_job_inlist5_name = 'undefined'

The canonical default inlist itself uses this to load other inlists.

Should we change these to arrays? I say yes. We can also scrap the logical flag and just use the empty string '' to indicate that an inlist should not be read (unless I'm missing a use case where this helpful). e.g. I imagine a user would write

    extra_star_job_inlist_name(1) = 'inlist_project'

and the defaults file containing

    extra_star_job_inlist_name(:) = ''

to cover all values that aren't used. E.g., in a loop like this should work:

do i=1, max_extra_inlists
   if (extra_star_job_inlist_name(i) == '') continue
   ... ! otherwise read it

If the logical flag is useful, we can instead use

do i=1, max_extra_inlists
   if (.not. read_extra_star_job_inlist(i)) continue
   ... ! otherwise read it

The only downside I can think of is that this will (trivially) break some backwards compatibility but I don't think that's a strong argument.

@warrickball warrickball added the good first issue Good for newcomers label Feb 7, 2022
@fxt44
Copy link
Member

fxt44 commented Feb 7, 2022

let me ask, what is gained by moving to arrays?

i suspect most of the test suite will need adjustments to make this work ...

@warrickball
Copy link
Contributor Author

warrickball commented Feb 7, 2022

let me ask, what is gained by moving to arrays?

We can refactor processing the inlists into loops, which at a rough count would eliminate ~300 lines of code. It also means that future changes only have to be applied once, not 5 times. Plus (though I don't think anyone has ever asked for more) it becomes trivial to increase the limit past 5.

i suspect most of the test suite will need adjustments to make this work ...

I think a few tactical seds will take care of almost all of these.

@fxt44
Copy link
Member

fxt44 commented Feb 8, 2022

fine by me. sharing said tactical sed scripts might be useful to some users.

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

No branches or pull requests

3 participants