Skip to content

Comments

updated default value listed in --maximum-evolution-time#1448

Open
FloorBroekgaarden wants to merge 5 commits intodevfrom
fix-minor-documentation-typo-max-timestep
Open

updated default value listed in --maximum-evolution-time#1448
FloorBroekgaarden wants to merge 5 commits intodevfrom
fix-minor-documentation-typo-max-timestep

Conversation

@FloorBroekgaarden
Copy link
Collaborator

there is a minor type in the documentation and YAML file for --maximum-evolution-time

the default YAML file says "maximum-evolution-time: 14021.28" (https://github.com/TeamCOMPAS/COMPAS/blob/dev/compas_python_utils/preprocessing/compasConfigDefault.yaml ) but the online documentation says:
--maximum-evolution-time
Maximum time to evolve binaries (Myr). Evolution of the binary will stop if this number is reached. Default = 13700.0

The correct value is 14031, which is the value that one obtains when you take the HUBBLE_TIME value from src/constants.h (and use the default definition from src/Options.cpp which defines maximum-evolution-time as: m_MaxEvolutionTime > HUBBLE_TIME / SECONDS_IN_MYR, which leads to 14031 Myr as the default (The 14021.28 Myr value comes from doing the same calculations but rounding values early on). More precisely, the calculation is: 1/ (67.8 * 1000.0 / (3.0* 10^(22))) seconds to Myr = 14031 Myr , with 67.8 from WMAP from the constant.h defaults.

Again, not a big deal, just a rounding of the value/typo. So a very minor fix

there is a minor type in the documentation and YAML file for --maximum-evolution-time

the default YAML file says "maximum-evolution-time: 14021.28"  (https://github.com/TeamCOMPAS/COMPAS/blob/dev/compas_python_utils/preprocessing/compasConfigDefault.yaml )
but the online documentation says:
--maximum-evolution-time
Maximum time to evolve binaries (Myr). Evolution of the binary will stop if this number is reached.
Default = 13700.0

The correct value is 14031, which is the value that one obtains when you take the  HUBBLE_TIME value from src/constants.h (and use the default definition from  src/Options.cpp which defines maximum-evolution-time as: `m_MaxEvolutionTime > HUBBLE_TIME / SECONDS_IN_MYR`,  which leads to 14031 Myr as the default  (The 14021.28 Myr value comes from doing the same calculations but rounding values early on). More precisely, the calculation is:  1/ (67.8 * 1000.0 / (3.0* 10^(22)))  seconds to Myr = 14031 Myr , with 67.8 from WMAP from the constant.h defaults.

Again, not a big deal, just a rounding of the value/typo. So a very minor fix
@FloorBroekgaarden FloorBroekgaarden added the documentation Improvements or additions to documentation label Jan 22, 2026
@github-actions
Copy link

✅ COMPAS Build Successful!

Item Value
Commit 88e7bea
Logs View workflow

Detailed Evolution Plot

Click to view evolution plot


Generated by COMPAS CI

@ilyamandel
Copy link
Collaborator

Thanks, @FloorBroekgaarden !
Though it really shouldn't be the Hubble time -- it should be the age of the Universe, and the Hubble time is only an approximation to that (the integral in https://en.wikipedia.org/wiki/Age_of_the_universe#Definition is not quite 1).
The WMAP age of the Universe was closer to 13.7 Gyr.
Can I suggest we use 13.8 Gyr everywhere, consistent with both the WMPA and Planck values (see, e.g., the same wikipedia page)?

@FloorBroekgaarden
Copy link
Collaborator Author

I like that solution/suggestion! This also reduced the number of calculations.
I'm currently traveling without a laptop but will push it when Im back (before Monday).

@ilyamandel
Copy link
Collaborator

@FloorBroekgaarden -- gentle ping :)

Copy link
Collaborator

@ilyamandel ilyamandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't actually changed the code, @FloorBroekgaarden :

Options.cpp: m_MaxEvolutionTime = HUBBLE_TIME / SECONDS_IN_MYR; //13700.0;

@FloorBroekgaarden
Copy link
Collaborator Author

whoops, thanks!
Do we want to keep COMPLAIN_IF(m_MaxEvolutionTime > 13800, "Maximum evolution time in Myr (--maxEvolutionTime) must be <= " + std::to_string(13800) + " Myr");
?

On one hand it might be nice to keep this to stop the simulation from running for COMPAS users who accidentally set the max evolution to a too large value, though, of course, it's technically not forbidden to evolve binaries beyond this max evolution time (e.g., if someone wants to calculate population characteristics for the future Universe).

@ilyamandel
Copy link
Collaborator

I can see arguments for dropping the check if the user knows what they want; on the other hand, the check prevents confused readers from accidentally taking too long while running slow binaries, as you say... Let's leave it for now, since we haven't run into a case when it's a problem yet. One final request: since you've now (rightly) edited the C++ code, you should increase the version number and add a comment in changelog.h.

Copy link
Collaborator

@jeffriley jeffriley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_MaxEvolutionTime is declared as a double on Options.h. It's good c++ coding practice to
set the value to 13800.0 in Options.cpp rather than just 13800.

Similarly, the value in the yaml file should be a floating point number, not an integer.

@ilyamandel
Copy link
Collaborator

Note that you should auto-generate the yaml file with --create-YAML-file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants