argiopetech / base

Bayesian Analysis for Stellar Evolution
http://webfac.db.erau.edu/~vonhippt/base9/
11 stars 4 forks source link

flags of carbonicity not working as expected #61

Closed sishijing closed 7 years ago

sishijing commented 7 years ago

When I set flags. --startingCarbonicity, --priorCarbonicity and --sigmaCarbonicity on the command line, the singlePopMcmc does not run at all. But I can run singlePopMcmc when I set values for these parameters in the base9.yaml file.

tedvh commented 7 years ago

Hi Max -

that makes great sense. Thanks for all this work.

By the way, while you are at it, can you change the default carbonicity from 0.0 to 0.38?

thanks, -Ted

On 1/5/17 6:49 AM, Max von Hippel wrote:

What's happening is that the code does not check to see if a value was already set in the command line args before setting it based on YAML. SO when you combine YAML with command line args your command line args are superseded but the YAML args. Since you haven't updated to the latest version, when it fails to find anything in the YAML, it crashes (see getOrDie in Settings.cpp). If you update to the latest version, it will not die, but instead ask you to re-enter the missing value. That said, this behavior is clearly redundant.

What I plan to do is figure out what the defaults are for each of the requested values and then use ternaries to only update values which have not yet been set. For example, according to Cluster.hpp, carbonicity is set by default to 0.0. So in Settings.cpp, I could do something like:

cluster.starting.carbonicity = cluster.starting.carbonicity ==0.0 ? getOrRequest (startingNode,"carbonicity") : cluster.starting.carbonicity;

Instead of

cluster.starting.carbonicity = getOrRequest (startingNode,"carbonicity");

Because I will have to find the default value for each of the values to be checked, implementing this will be a bit time intensive, so I'll probably wait to do it until a bit later this week. If possible I hope to also improve the prompt for variable entree using node name (for example, "please enter your desired general.verbose.cluster.starting.carbonicity" is much more helpful than "please enter your desired carbonicity" as there are multiple carbonicities to be entered).

Hope that makes sense!

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/argiopetech/base/issues/61#issuecomment-270576195, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOeMhhww7CDb6c1Hq3M3FEMmE2rPltWks5rPJKKgaJpZM4LbOXc.

maxvonhippel commented 7 years ago

@tedvh should default carbonicity be 0.38 for all carbonicity values (cluster.starting. carbonicity, cluster.priors.means. carbonicity, and cluster.priors.sigmas. carbonicity)? Also, does this mean I should change the default for singlePopMcmc.stepSizes.carbonicity?

tedvh commented 7 years ago

Hi Max -

good question. The defaults should be

0.38 for cluster.starting.carbonicity and 0.38 for cluster.priors.means.carbonicity

yet

0.0 for cluster.priors.sigmas.carbonicity

I don't know about singlePopMcmc.stepSizes.carbonicity . That is a tuning parameter and should probably be left wherever Elliot put it for now.

-Ted

On 1/6/17 12:21 AM, Max von Hippel wrote:

@tedvh https://github.com/tedvh should default carbonicity be 0.38 for all carbonicity values (|cluster.starting. carbonicity|, |cluster.priors.means. carbonicity|, and |cluster.priors.sigmas. carbonicity|)? Also, does this mean I should change the default for |singlePopMcmc.stepSizes.carbonicity|?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/argiopetech/base/issues/61#issuecomment-270796944, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOeMtVBYyKAazojYS7UONUyuC6Olq4oks5rPYj0gaJpZM4LbOXc.

maxvonhippel commented 7 years ago

Sounds good, thanks.

maxvonhippel commented 7 years ago

I take back my comment about logging, I decided the approach I'd taken was too complex so I redid it to just address the particular problem raised here and not try to tackle logging simultaneously. I'll work on that next in another pr.

tedvh commented 7 years ago

Hi Max -

Good eye. Sure, go ahead and make that variable name change. We have changed the burnin techniques a few times an probably that didn't propagate.

Elliot can also chime in if he recognizes a problem related to this.

-Ted

On 1/6/17 3:57 AM, Max von Hippel wrote:

@tedvh https://github.com/tedvh for some reason the Node |singlePopMcmc.burnIter| is called |stage2IterMax| in the YAML file. Every single other variable is named the same thing in the code and the YAML. Can I go ahead and change it to use |burnIter| in the YAML? (Doing so would make my code easier to write, and I think it's more logical to use the same thing in both places anyway.)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/argiopetech/base/issues/61#issuecomment-270825897, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOeMlMVes59cAgv-i3qrIujMCmRM7bYks5rPbvCgaJpZM4LbOXc.

tedvh commented 7 years ago

OK, sounds good.

On 1/6/17 6:44 AM, Max von Hippel wrote:

I take back my comment about logging, I decided the approach I'd taken was too complex so I redid it to just address the particular problem raised here and not try to tackle logging simultaneously. I'll work on that next in another pr.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/argiopetech/base/issues/61#issuecomment-270842856, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOeMtfmqBxICJ0vH4-N0MqUXtq9x_FKks5rPeLRgaJpZM4LbOXc.

maxvonhippel commented 7 years ago

I deleted my comment about burnin because I found about 4 other variables with name mis-matches. I want to make the names be the same in the YAML, the code, and the command line args, with only a slight difference in that YAML is of form

path.to.node[key] = variable

... and command line args look like:

--pathToNodeKey=variable

... and c++ variables are a bit abstracted, along the lines of:

// sudo code
Node config = whatever;
Node whiteDwarf = some stuff to get whiteDwarf from config;
variable whiteDwarf.someSpecificMetric = getOrRequest(... );

Anyway, I plan to go through and document all of the name mismatches and then open a new issue with them. Then you and Elliot can decide what names to use throughout and I will update it accordingly.

maxvonhippel commented 7 years ago

Also @tedvh you can close this issue now.

tedvh commented 7 years ago

sounds like a good plan.

On 1/6/17 4:04 PM, Max von Hippel wrote:

I deleted my comment about burnin because I found about 4 other variables with name mis-matches. I want to make the names be the same in the YAML, the code, and the command line args, with only a slight difference in that YAML is of form

path.to.node[key] = variable

... and command line args look like:

--pathToNodeKey=variable

... and c++ variables are a bit abstracted, along the lines of:

// sudo code Node config = whatever; Node whiteDwarf = some stuff to get whiteDwarf from config; variable whiteDwarf.someSpecificMetric = getOrRequest(... );

Anyway, I plan to go through and document all of the name mismatches and then open a new issue with them. Then you and Elliot can decide what names to use throughout and I will update it accordingly.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/argiopetech/base/issues/61#issuecomment-270936040, or mute the thread https://github.com/notifications/unsubscribe-auth/AEOeMv6FypEJUBEsPDgjglIVV0NJSybEks5rPmX1gaJpZM4LbOXc.