search

TeamCOMPAS / COMPAS

COMPAS rapid binary population synthesis code
http://compas.science
MIT License
64 stars 64 forks source link

New error handling #1151

Closed jeffriley closed 2 days ago

jeffriley commented 1 week ago

@ilyamandel there is a lot going on in this PR:

  1. implementation of more coherent and robust error handling
  2. added source files (all are .h file, so the makefile does not need to change)
  3. (this will be controversial) deprecation of some program options, and some program option values
  4. Added SN type SNII and supporting functions etc. The code as is is dysfunctional for typeIIA SN (I know they're a made-up type, but we call the function to resolve one about 0.7% of the time - we need to resolve that). For now, what I've implemented resolves it - we can change it if it's wrong.
  5. Added debug functionality to show stack trace and halt the program - see the discussion and implementation of the SIGUSR2 signal handler in main.cpp
  6. Fixed what I believe was a defect in utils::SolveKeplersEquation() that was causing erroneous "out-of-bounds" warnings for the eccentric anomaly
  7. code cleanup
  8. I have not changed the online docs - I wanted to push this so you could start reviewing it - I will update the docs as you complete your review
  9. I updated changelog.h, but I haven't fleshed it out yet - I won't do that until after your review so I know what's staying, what's being dropped, and what's changing
    1. I have not created a new default yaml file - I won't do that until after your review
    2. I advanced the version number to v03.00.00. I think the new error handling philosophy is enough of a change to the COMPAS functionality to warrant bumpin the major number (let me know if you don't agree)

Doesn't sound much when I write it like that...

  1. The new error handling philosophy is described at the top of the Errors.h file.

  2. The added source files are mostly the result of me separating out sections of the constants.h file. IMO it had become too unwieldy, and sectioning it out seemed to be the reasonable thing to do. I broke constants.h into 5 separate file:

    a. constants.h - pretty-much just contains constants now b. typedefs.h - an existing file, but I moved the enum class declarations and associated label maps, except those that pertain directly to logging, from constants.h to typedefs.h c. LogTypedefs.h - new file containing logging-related type definitions, including things like definitions of the default record composition for the various log files d. ErrorCatalog.h - new file containing the COMPAS error catalog - this where symbolic names for errors are defined, and contains the mapping from symbolic name to error string e. EnumHash.h - contains the hash function for enum class types

    Other added files are (only 1):

    a. stellarUtils.h - similar to utils.h but includes the stellar classes (and header files) so that functions are aware of the stellar types (not possible in utils.h because that file is includes in the stellar classes - the includes would become circular)

  3. As I was implementing the new error handling and doing some code cleanup, I noticed that some of our program options had been implemented with names inconsistent with existing option names. As well, some of the option values (imo) didn't represent what the value actually meant (I may well be corrected on some, or all, of those - happy to revert my changes there if that's the case). I know we don't like to change too much that is user-facing, but I think it is important we are consistent with the user-facing stuff...

I have changed some option names, and the names of some option values - effectively deprecating some options and option values. I have left the deprecated options and option values in the code, alongside their replacements (if applicable). I have written code (in the Options class) to notify users when they use the deprecated options and option values that they are indeed deprecated and will be removed soon, and where applicable showing the replacement option name or option value name. The code doesn't require there be a replacement, and where's there's not one, that part of the deprecation notice is not displayed.

The options I deprecated are:

   1. `--mass-transfer`                       , replaced by `--use-mass-transfer` (to be consistent with `--use-mass-loss`)
   2. `--luminous-blue-variable-prescription` , replaced by `--LBV-mass-loss-prescription` (to be consistent with other 'prescription'-type options
   3. `--OB-mass-loss`                        , replaced by `--OB-mass-loss-prescription` (to be consistent with other 'prescription'-type options
   4. `--RSG-mass-loss`                       , replaced by `--RSG-mass-loss-prescription` (to be consistent with other 'prescription'-type options
   5. `--VMS-mass-loss`                       , replaced by `--VMS-mass-loss-prescription` (to be consistent with other 'prescription'-type options
   6. `--WR-mass-loss`                        , replaced by `--WR-mass-loss-prescription` (to be consistent with other 'prescription'-type options
   7. `--kick-direction`                      , replaced by `--kick-direction-distribution` (to be consistent with other 'distribution'-type options
   8. `--mass-transfer-thermal-limit-accretor`, replaced by `--mass-transfer-thermal-limit-accretor-multiplier` (for consistency and to better describe the option)
   9. `--black-hole-kicks`                    , replaced by `--black-hole-kicks-mode` (for consistency and to better describe the option) 
  10. `--chemically-homogeneous-evolution`    , replaced by `--chemically-homogeneous-evolution-mode` (for consistency and to better describe the option)

I know changing these could be problematic, but if we're ever going to (and I think we should), now is the time to do it. I should note that the deprecated options will still work in the deprecation notice period, and even after they are removed many will continue to work because Boost only requires sufficient characters to match to uniquely identify an option.

The option values deprecated are:

   1. `--critical-mass-ratio-prescription`   , value `NONE` replaced by `ZERO`
   2. `--LBV-mass-loss-prescription`         , value `NONE` replaced by `ZERO`
   3. `--logfile-type`                       , value `NONE` replaced by `ZERO`
   4. `--luminous-blue-variable-prescription`, value `NONE` replaced by `ZERO`
   5. `--OB-mass-loss`                       , value `NONE` replaced by `ZERO`
   6. `--OB-mass-loss-prescription`          , value `NONE` replaced by `ZERO`
   7. `--RSG-mass-loss`                      , value `NONE` replaced by `ZERO`
   8. `--RSG-mass-loss-prescription`         , value `NONE` replaced by `ZERO`
   9. `--stellar-zeta-prescription`          , value `NONE` replaced by `ZERO`
  10. `--VMS-mass-loss`                      , value `NONE` replaced by `ZERO`
  11. `--VMS-mass-loss-prescription`         , value `NONE` replaced by `ZERO`
  12. `--WR-mass-loss`                       , value `NONE` replaced by `ZERO`
  13. `--WR-mass-loss-prescription`          , value `NONE` replaced by `ZERO`

The motivation for changing these is that (imo) ZERO is a better description of the option value. For example, NONE implies no (whatever) prescription, when what we really mean is that if the user chooses NONE we'll apply a value of 0.0 - to me that's better described by a prescription value of ZERO than NONE. As I noted above though, I'm happy to be corrected and revert any or all of these.

As an example of the deprecation functionality, running COMPAS with the following commandline:

   ./compas --mode bse --random-seed 0 --logfile-type csv --fp-error-mode off --remnant-mass-prescription schneider2020 --mass-transfer --grid gridfile.txt --OB-mass-loss-prescription NONE --critical-mass-ratio-p none

and gridfile.txt:

   --initial-mass 12.3 --metallicity 0.02
   --initial-mass 1.3 --metallicity 0.012
   --initial-mass 22.3 --metallicity 0.015
   --initial-mass 32.5 --metallicity 0.01 --vms-mass-loss VINK2011
   --initial-mass 2.3 --metallicity 0.02
   --initial-mass 112.3 --metallicity 0.014
   --initial-mass 125.3 --metallicity 0.01 --rsg-mass-loss NONE

results in the following output:

   COMPAS v03.00.00
   Compact Object Mergers: Population Astrophysics and Statistics
   by Team COMPAS (http://compas.science/index.html)
   A binary star simulator

   Go to https://compas.readthedocs.io/en/latest/index.html for the online documentation
   Check https://compas.readthedocs.io/en/latest/pages/whats-new.html to see what's new in the latest release

   DEPRECATION NOTICE: option '--mass-transfer' has been deprecated and will soon be removed.  Please use '--use-mass-transfer' in future.
   DEPRECATION NOTICE: option value 'NONE' for option '--critical-mass-ratio-prescription' has been deprecated and will soon be removed.  Please use 'ZERO' in future.
   DEPRECATION NOTICE: option value 'NONE' for option '--OB-mass-loss-prescription' has been deprecated and will soon be removed.  Please use 'ZERO' in future.

   Start generating binaries at Mon Jun 24 19:44:15 2024

   0: Allowed time exceeded: (Main_Sequence_>_0.7 -> Neutron_Star) + (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf)
   1: Double White Dwarf formed: (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf) + (Main_Sequence_>_0.7 -> Helium_White_Dwarf)
   2: Double White Dwarf formed: (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf) + (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf)
   DEPRECATION NOTICE: option '--VMS-mass-loss' has been deprecated and will soon be removed.  Please use '--VMS-mass-loss-prescription' in future.
   3: Double White Dwarf formed: (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf) + (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf)
   4: Stars merged: (Main_Sequence_>_0.7 -> Naked_Helium_Star_MS) + (Main_Sequence_>_0.7 -> Naked_Helium_Star_MS)
   5: Double White Dwarf formed: (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf) + (Main_Sequence_>_0.7 -> Oxygen-Neon_White_Dwarf)
   DEPRECATION NOTICE: option '--RSG-mass-loss' has been deprecated and will soon be removed.  Please use '--RSG-mass-loss-prescription' in future.
   DEPRECATION NOTICE: option value 'NONE' for option '--RSG-mass-loss' has been deprecated and will soon be removed.  Please use 'ZERO' in future.
   6: Double White Dwarf formed: (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf) + (Main_Sequence_>_0.7 -> Carbon-Oxygen_White_Dwarf)

   Generated 7 of 7 binaries requested

   Simulation completed

   End generating binaries at Mon Jun 24 19:44:15 2024

As I was documenting this PR I realised there are a couple of program option property types (noted in enum class PROGRAM_OPTION and associated label map in LogTypedefs.h that will be deprecated but that Options::ShowDeprecations() won't display. Not sure yet whether just noting those on the 'What's New' page is enough, or whether I should write code to show the deprecation notice if they are specified (they are for the logfile-definitions file). I'm leaning toward the latter.

Also, when testing the new --fp-error-mode option I found a divide-by-zero in HG::Initialise. CalculateTimescales() calls GiantBranch::CalculateTimescales(), which has:

   double p1_p = p1 / gbParams(p);

which caused a floating-point divide-by-zero (because gbParams(p) initial value is 0.0). I fixed it by adding a call to CalculateGBParams() just prior to the call to "CalculateTimescales()" in HG::Initialise. However, you will see, if you test the --fp-error-mode option that there are many more errors throughout the code - almost every star or binary reports a floating-point error...

Code cleanup: There is a lot of code cleanup - sorry. I have identified problems or questions that need an answer by including the text "**ilya**" at the appropriates places, so that you can search for that string and answer, or at least offer an opinion. The rest you could probably just take a cursory look. I think downloading the source and just glancing at each file is probably going to easier than looking at the diffs...

And finally, I know we discussed removing the BE Binary code at some stage - what did we decide?

I think that's everything - if I remember something else I'll add it. I confess I rushed a bit today to get this submitted this evening - I'm conscious that you don't have a lot of time to review this.

And now, for the unreasonable request: I'd really like to get this merged before I leave for Italy which, I think, means we need to get it merged before you leave for your vacation at the end of this week...

jeffriley commented 1 week ago

I meant to add:

  1. I used "soon" in the deprecation notice to give us some flexibility with when we actually remove a deprecated option or value. We could use a fixed date instead to give users a better idea of how long they have to change their configurations. Perhaps something like "... will be removed no earlier than Dec 31, 2024", or "... will be removed on or after Dec 31, 2024".
  2. If a user specifies both a deprecated option, and its replacement, on the same commandline or gridline, the value for the non-deprecated option is used.
  3. As you could probably tell from the sample output, notices for deprecated options on the commandline appear immediately after the splash message, and notices for deprecated options on gridlines are shown as they are encountered.