Develop coding style guide

[jschwab]

It would be useful to have a coding style guide. This could cover information about formatting as well as style (do loops vs arrays).

[warrickball]

Definitely useful, also to make it easier to review code in pull requests. If we can press for contributed code to at least adhere to some reasonable practices it'll be easier to review and also help to maintain the standard of the code.

I don't think it's hard to throw together a lot of what "MESA style" means but there are some higher-level things to consider. How specific should we be/do we want to be? (e.g. do we care how many spaces are used to indent lines? do we allow lines containing only whitespace?) How strictly will we want code to adhere? Strict adherence to a specific style seems hypocritical given that the code is currently not particularly uniform. We could, of course, try to make it uniform. There would be a lot of commit churn but the switch from SVN to Git is a good opportunity to do a lot of that without it polluting the commit history. (I'm not proposing we do this.)

If we are very specific, we could probably put together a linter (or extend @rjfarmer's).

Personally, I'm really not bothered about the finer points (e.g. I don't mind how much indentation is used or if it's inconsistent) but there are things I would want to point out to contributors. Some are just good practice that I'm not sure are commonly taught (in other words, I learned them from MESA and GYRE):

• Explicit intent. Procedure variables should have and explicit intent(in), intent(out) or intent(inout) attributes.
• Explicit imports (when reasonable). If you're only using a few subroutines or variables from another module, import with use, only: ... rather than use. If you're only using them in a particular procedure, put the use statement in that procedure.

Some are MESA specific:

• Use the const module. You don't need to redefine π, etc.
• Bit-for-bit reproducibility. Use math_lib. Use the dp variable from const_def and declare floating point variables as real(dp). Write constants in expressions as doubles using either 0.3467d0 or 0.3467_dp (or do we want to pick one?). Prefer the pow function even for integer powers (or use e.g. x*x instead of x**2).
• Legacy code. There are some parts of MESA that bundle code ported from Fortran 77-style packages. I think the consensus is that these represent a "not broken, don't fix" situation.
• Loops vs arrays. A lot of operations that could be written as array operations are explicitly written as loops because writing them as arrays caused problems before.

Although it probably belongs in a "Contributing" section rather than a style guide, the first thing we might want to ask new contributors is whether their code really needs to be merged into the trunk or whether it can be achieved using run_star_extras.f.

[rjfarmer]

How specific should we be/do we want to be? (e.g. do we care how many spaces are used to indent lines?

3 spaces (Bill cares)

How strictly will we want code to adhere?

There's different levels (maybe), bit-for-bit is a must for instance, while some things might be "this is our preferred style".

Strict adherence to a specific style seems hypocritical given that the code is currently not particularly uniform.

I think we just say that all new code should follow the standard (old code should just be updated as we work on it for other reasons)

the first thing we might want to ask new contributors is whether their code really needs to be merged into the trunk or whether it can be achieved using run_star_extras.f.

We can write some github templates (https://help.github.com/en/github/building-a-strong-community/about-issue-and-pull-request-templates) that ask the user this question when they submit a issue/pull request.

[rhdtownsend]

On Oct 25, 2019, at 9:34 AM, Robert Farmer notifications@github.com wrote:

How specific should we be/do we want to be? (e.g. do we care how many spaces are used to indent lines?

3 spaces (Bill cares)

Agreed. To help with this sort of thing, we can provide editor config files (e.g., .emacs settings) that set up things like this.

How strictly will we want code to adhere?

There's different levels (maybe), bit-for-bit is a must for instance, while some things might be "this is our preferred style".

There are also certain language adherence requirements -- e.g., standard Fortran (03? 08? 18?).

Strict adherence to a specific style seems hypocritical given that the code is currently not particularly uniform.

I think we just say that all new code should follow the standard (old code should just be updated as we work on it for other reasons)

the first thing we might want to ask new contributors is whether their code really needs to be merged into the trunk or whether it can be achieved using run_star_extras.f.

My stance over the last 6 months has been 'if I change this existing file significantly, then it behooves me to adopt the style I've used elsewhere for new files'. So, old stuff is grandfathered in, but once it gets changed it must be updated.

cheers,

R

We can write some github templates (https://help.github.com/en/github/building-a-strong-community/about-issue-and-pull-request-templates) that ask the user this question when they submit a issue/pull request.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[jschwab]

There's a spot for this info at docs/source/developing/code_style.rst.

[adamjermyn]

I just looked through code_style.rst and have two questions:

1. autodiff variables work just fine with ** and with pow. Are we okay allowing ** with autodiff types, or do we want to insist on uniformity here? If we want uniformity I can disable ** on autodiff so the compiler enforces it :-)
2. In the errors section are we saying that all subroutines have to return ierr? We have routines that do things that can't fail unless something earlier failed (e.g. take this number and do some basic arithmetic to it, which can't produce NaN unless an input was NaN).

[rhdtownsend]

On Oct 28, 2020, at 12:18 PM, Adam Jermyn notifications@github.com wrote:

I just looked through code_style.rst and have two questions:

• autodiff variables work just fine with and with pow. Are we okay allowing with autodiff types, or do we want to insist on uniformity here? If we want uniformity I can disable ** on autodiff so the compiler enforces it :-)

I suggest we insist on uniformity -- when reading code, I shouldn't have to check whether a variable is autodiff or intrinsic, in order to know whether usage of ** is allowed or not.

• In the errors section are we saying that all subroutines have to return ierr? We have routines that do things that can't fail unless something earlier failed (e.g. take this number and do some basic arithmetic to it, which can't produce NaN unless an input was NaN).

On a similar note, why is ierr intent(inout) rather than intent(out)? Should a subroutine be expected to work properly if a non-zero ierr is passed in?

And do we want to distinguish different ierr values? Positive for invalid arguments, negative for a failure that occurred even with valid arguments?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[rjfarmer]

I agree with @rhdtownsend we want consistency with and pow (given the number of times we keep having the discussion on mesa-dev as it whether somethingsomething is/not bit for bit)

In the errors section are we saying that all subroutines have to return ierr?

No i was just trying to show the standard way we should be doing error checking in the places were we do error checking (i.e if you are returning an error code don't call it anything other than ierr) and if you can't handle the error call mesa_error()

On a similar note, why is ierr intent(inout) rather than intent(out)? Should a subroutine be expected to work properly if a non-zero ierr is passed in? And do we want to distinguish different ierr values? Positive for invalid arguments, negative for a failure that occurred even with valid arguments?

I have no preference on inout vs out. Whether positive or negative have meaning we would first need to check if we do this anywhere now (or even bother doing anything other than set -1)

[rhdtownsend]

On Oct 28, 2020, at 2:08 PM, Robert Farmer notifications@github.com wrote:

I agree with @rhdtownsend we want consistency with and pow (given the number of times we keep having the discussion on mesa-dev as it whether somethingsomething is/not bit for bit)

In the errors section are we saying that all subroutines have to return ierr?

No i was just trying to show the standard way we should be doing error checking in the places were we do error checking (i.e if you are returning an error code don't call it anything other than ierr) and if you can't handle the error call mesa_error()

On a similar note, why is ierr intent(inout) rather than intent(out)? Should a subroutine be expected to work properly if a non-zero ierr is passed in? And do we want to distinguish different ierr values? Positive for invalid arguments, negative for a failure that occurred even with valid arguments?

I have no preference on inout vs out. Whether positive or negative have meaning we would first need to check if we do this anywhere now (or even bother doing anything other than set -1)

In this case there is a semantic difference between inout and out -- the former implies that the operation of the subroutine could be influenced by the input value of ierr. If we don't want this implication, we should mandate that ierr is intent(out).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[adamjermyn]

I think we do want subroutines to be able to read ierr (e.g. so they can halt if ierr /= 0), so if I've understood correctly we should make ierr standardized to inout.

Regarding autodiff: I'll remove support for ** then.

[rhdtownsend]

On Oct 28, 2020, at 3:39 PM, Adam Jermyn notifications@github.com wrote:

I think we do want subroutines to be able to read ierr (e.g. so they can halt if ierr /= 0), so if I've understood correctly we should make ierr standardized to inout.

But shouldn't it fall to the calling routine to inspect ierr, and not pass it into a subroutine if its non-zero (since the subroutine will have no idea what to do with ierr/=0)?

cheers,

R

Regarding autodiff: I'll remove support for ** then.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[warrickball]

And do we want to distinguish different ierr values? Positive for invalid arguments, negative for a failure that occurred even with valid arguments?

I think this is a whole other discussion (that we should have). I expect we want to plan (and document) this quite carefully because if we decide on specific error codes in even a few subroutines and have the calling functions respond appropriately, the error codes might start percolating throughout MESA and become hard to undo.

[rjfarmer]

@rhdtownsend sometimes you may want to bundle all the error handling into its own subroutine which you also pass ierr in as an argument, but i also don't want to spend forever debating what is a very minor point.

[adamjermyn]

Is there more that needs to happen here, or can we close this issue?

[rjfarmer]

Yes there is , as we have only added some things to the docs before getting distracted by intent(inout) vs out for ierr. There are presumably more things we would like to say is the way we do things.

[matthiasfabry]

I'd like to bump this by saying lots of .f90 files aren't formatted free-form style, but rather fixed-form style. I'd argue free-from dramatically improves readability, e.g. look at star/public/star_def.f90:

      module star_def

      use star_data_def

      implicit none      

      end module star_def

vs.

module star_def

   use star_data_def

   implicit none

end module star_def

I might be overly fanatic about this, but I feel the code base could benefit from this. I can try a bulk reformat if this is something we'd like to see happen.

[rhdtownsend]

I wholeheartedly approve of such a change; much of the current source code is (visually) an abomination.

On Apr 17, 2023, at 5:23 AM, Matthias Fabry @.***> wrote:

I'd like to bump this by saying lots of .f90 files aren't formatted free-form style, but rather fixed-form style. I'd argue free-from dramatically improves readability, e.g. look at star/public/star_def.f90: module star_def

use star_data_def

implicit none

end module star_def

vs. module star_def

use star_data_def

implicit none

end module star_def

I might be overly fanatic about this, but I feel the code base could benefit from this. I can try a bulk reformat if this is something we'd like to see happen. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[rjfarmer]

@matthiasfabry if you are going to insist on whole codebase changes please only change the Fortran files. I've seen your free-format-formatting branch and you are changing every file including those that aren't Fortran files. It's going to be difficult to merge if you insist on changing every file at once. Instead, go for one module at a time so there is even a slight chance we can review changes before merging.

Please also don't change the code in the stella folder. No one here can fix things if you break something in it.

[matthiasfabry]

yes, that makes sense from a review standpoint. I can restart and make a folder of branches.

Also good to know stella is off limits

[matthiasfabry]

I've created formatting/astero, /auto_diff, /binary and /const for you to review. I've tried to be as careful as possible for the formatter to only touch .f90 files.

[evbauer]

The fortran code in auto_diff is actually generated by python scripts from the python directory in that module, so we should generally avoid direct edits to the auto_diff/private files. If you need to make edits to auto_diff, you should edit python files and then regenerate the fortran as necessary.

I guess this isn't particularly well documented either, so maybe we should try to add some documentation somewhere to help developers navigate editing auto_diff if necessary. I'll admit I'm not very clear on that at the moment.

[earlbellinger]

Here's some documentation on auto_diff: https://adamjermyn.com/posts/auto_diff_mesa/

Best wishes, Earl

On Thu, Apr 20, 2023 at 4:46 PM Evan Bauer @.***> wrote:

The fortran code in auto_diff is actually generated by python scripts from the python directory in that module, so we should generally avoid direct edits to the auto_diff/private files. If you need to make edits to auto_diff, you should edit python files and then regenerate the fortran as necessary.

I guess this isn't particularly well documented either, so maybe we should try to add some documentation somewhere to help developers navigate editing auto_diff if necessary. I'll admit I'm not very clear on that at the moment.

— Reply to this email directly, view it on GitHub https://github.com/MESAHub/mesa/issues/76#issuecomment-1516461308, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANMPS3DKAUSXMVNMTSQZ6TXCFD4RANCNFSM4PXCDVCA . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[evbauer]

Nice! I didn't realize there was so much nice documentation out there. We probably shouldn't rely too heavily on @adamjermyn's blog for hosting important documentation though, so I wonder if there's an easy way to pull that page into the MESA website as part of our auto_diff documentation page here.

@adamjermyn do you happen to have source that would be compatible the the ReST format of MESA docs? Or should we just start by copying over the html?

[adamjermyn]

The blog post is auto-generated with Hugo from the attached markdown file. Should be easy enough to get it into ReST format!

-Adam

On Thu, Apr 20, 2023 at 12:20 PM, Evan Bauer @.***> wrote:

Nice! I didn't realize there was so much nice documentation out there. We probably shouldn't rely too heavily on @adamjermyn https://github.com/adamjermyn's blog for hosting important documentation though, so I wonder if there's an easy way to pull that page into the MESA website as part of our auto_diff documentation page here https://docs.mesastar.org/en/latest/auto_diff/overview.html.

@adamjermyn https://github.com/adamjermyn do you happen to have source that would be compatible the the ReST format of MESA docs? Or should we just start by copying over the html https://github.com/adamjermyn/adamjermyn.github.io/blob/master/posts/auto_diff_mesa/index.html ?

— Reply to this email directly, view it on GitHub https://github.com/MESAHub/mesa/issues/76#issuecomment-1516612083, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPR5H3WZ5RM2QLMUJHELBLXCFO6HANCNFSM4PXCDVCA . You are receiving this because you were mentioned.Message ID: @.***>

[evbauer]

Thanks @adamjermyn! A markdown file should be super easy to port over. I don't see the attachment though...

[evbauer]

Got the attachment from Adam offline, and the markdown file was easy to convert over to restructured text, so that documentation now lives here on the MESA website: https://docs.mesastar.org/en/latest/auto_diff/more_detail.html

Sorry for the long tangent on auto_diff! We can go back to discussing the main point about coding style.

[fxt44]

awesome! thank you evan.