Skip to content

Fix to non-random supernova kick angles#1481

Merged
jeffriley merged 3 commits into
TeamCOMPAS:devfrom
AldanaGrichener:patch-1
May 26, 2026
Merged

Fix to non-random supernova kick angles#1481
jeffriley merged 3 commits into
TeamCOMPAS:devfrom
AldanaGrichener:patch-1

Conversation

@AldanaGrichener
Copy link
Copy Markdown
Contributor

Ran into another bug similar to issue #1378 (while working on the CBD PR, still forthcoming 🙂), applied the analogous fix to PR #1387.

(Missed it back then because I bypassed #1378 by using a bash script which sent each seed individually)

Ran into another bug similar to issue TeamCOMPAS#1378 (while working on the CBD PR, still forthcoming 🙂), applied the analogous fix to PR TeamCOMPAS#1387.

(Missed it back then because I bypassed TeamCOMPAS#1378 by using a bash script which sent each seed individually)
Copy link
Copy Markdown
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.

@jeffriley , I am a bit puzzled by the description of OPT_VALUE. It says:

//. if the user did not speify a value on
// the commandline, the commandline value is set according to the default
// behaviour for the option, and the grid line value is set from that. Note
// that for options whose default behaviours is to draw a random number, this
// will only be done once for the commandline value, and each grid line that
// falls back to the commandline will take the same value as the commandline.
// Consider using 'fallback' = 'false' for those cases.

[aside: note typo in speify, should be specify :) ]

My question is, how is the programmer to know whether this is the first time that OPT_VALUE is called for this option, and so whether a random-number should be regenerated or not?

@ilyamandel ilyamandel requested a review from jeffriley May 26, 2026 02:41
@jeffriley
Copy link
Copy Markdown
Collaborator

jeffriley commented May 26, 2026

My question is, how is the programmer to know whether this is the first time that OPT_VALUE is called for this option, and
so whether a random-number should be regenerated or not?

If I understand the question correctly @ilyamandel, the value for the option is set (once) at parse time in Options.cpp (and that's where we call RAND->Random()), OPT_VALUE just retrieves the value when called.

(EDIT: So the programmer doesn't need to know how many times the getter (which uses OPT_VALUE) is called - it will always return the same value.)

Copy link
Copy Markdown
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.

Looks good.

I fixed the typo :-)

Copy link
Copy Markdown
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.

I changed my mind. changelog needs to be updated - record the change and change the patch/fix number.

@ilyamandel
Copy link
Copy Markdown
Collaborator

@jeffriley : The comment says:
// Note
// that for options whose default behaviours is to draw a random number, this
// will only be done once for the commandline value, and each grid line that
// falls back to the commandline will take the same value as the commandline.
// Consider using 'fallback' = 'false' for those cases.
This code said exactly that -- false -- before Aldana's fix. Why was this the wrong code, given the code comment?

@jeffriley
Copy link
Copy Markdown
Collaborator

jeffriley commented May 26, 2026

@ilyamandel

This code said exactly that -- false -- before Aldana's fix. Why was this the wrong code, given the code comment?

Is one of us looking at the wrong changes? Didn't Aldana change the fallback values from "true" to "false"?

Or am I completely misunderstanding your question?

@ilyamandel
Copy link
Copy Markdown
Collaborator

You are right, @jeffriley . :)

Copy link
Copy Markdown
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.

Thanks @AldanaGrichener !

@AldanaGrichener
Copy link
Copy Markdown
Contributor Author

You are welcome :)

@jeffriley jeffriley merged commit 9358969 into TeamCOMPAS:dev May 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants