Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust Av and Age priors, allow verify_params failures #124

Merged
merged 7 commits into from
Nov 8, 2017

Conversation

lcjohnso
Copy link
Member

Here I enable three optional features:

  1. Add an exponential Av prior model.
  2. Allow the removal of the "constant in linear age" prior to prefer the grid's intrinsic logarithmic spacing.
  3. Allow verify_params failures to print to screen, but continue without exiting. This is helpful when using non-fA dust curves and fixed R_V values.

While not included in this PR, I would be happy to add info about what needs to be changed in run_beast.py and datamodel.py to use these updates, but I'll wait until the datamodel.py documentation page is drafted (Issue #71).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.003%) to 12.167% when pulling 7546400 on lcjohnso:adjust_priors_lcj into 6d93d78 on BEAST-Fitting:master.

@karllark
Copy link
Member

In the verify_params changes, I'm not sure having the exit being overridden for all possible exits is a good idea. In fact, ideally the override would not be done at all, but the verify_params logic changed to handle the two cases mentioned - no fA and/or fixed Rv extinction curves. @lcjohnso: Is such a change in the logic not possible/straightforward?

@lcjohnso
Copy link
Member Author

Re: @karllark's suggestion, I added logic to verify_params that allows for non-exit() warnings in the case that:

  1. fAs = None, as needed for case where non-Gordon16 extinction curve is used.

  2. Parameter grids are defined as single values (e.g., defining sequence is [0.0, 0.0, 0.1] leading to single grid point at 0.0).

I'm leaving the noexit option implemented (could still have use in future), but its use is discouraged as it might allow non-physical grid/parameter values.

@lcjohnso
Copy link
Member Author

Also: I added initial documentation of datamodel.py/run_beast.py, including HowTo instructions for implementing new optional features.

This addition is a good step towards closing Issue #71 -- please comment there on additions that might still be needed before closing issue.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.162% when pulling 0938598 on lcjohnso:adjust_priors_lcj into 6d93d78 on BEAST-Fitting:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 12.162% when pulling 7c17b2d on lcjohnso:adjust_priors_lcj into 6d93d78 on BEAST-Fitting:master.

x: vector
x values
a: float
Single parameter in exp: a*e^-ax
Copy link
Member

@karllark karllark Oct 19, 2017

Choose a reason for hiding this comment

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

This should be N*e^-ax.
It would be nice if the 'a' parameter had a variable name that was more descriptive. I can't think of one offhand, but it is not some kind of timescale?

Copy link
Member Author

Choose a reason for hiding this comment

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

I corrected the notation, but am leaving the param as "a" -- could use "decay_rate" or "decay_param" but that seems overkill. Instead, I added additional text in docstring.

x values
a: float
Single parameter in exp: a*e^-ax
N: floats
Copy link
Member

Choose a reason for hiding this comment

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

Is this parameter needed? Shouldn't this be set by the desire to have this prior integrate to 1 over the min/max age range? If so, then it is not needed and should be set inside the function.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following the format of the other prior choices. Will leave as is.


def verify_one_input_format(param, param_name, param_format, param_lim):
if param_max-param_min == 0.:
print('Warning: '+param_name+' grid is single-valued.')
Copy link
Member

Choose a reason for hiding this comment

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

Nit picky point. I don't think this is a warning. There is no problem with single value parameters in a grid. I worry that having it issue a warning makes it sound like this is bad. Maybe something less "scary" like "Info"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to "Note:"

@@ -0,0 +1,73 @@
Running the BEAST
Copy link
Member

Choose a reason for hiding this comment

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

Great to have this doc file! Many thanks for starting it. I know there is material I will add once I think about it more.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 12.153% when pulling 6e02d2d on lcjohnso:adjust_priors_lcj into 6d93d78 on BEAST-Fitting:master.

@karllark karllark merged commit 7d1e4ab into BEAST-Fitting:master Nov 8, 2017
@lcjohnso lcjohnso deleted the adjust_priors_lcj branch September 18, 2018 22:12
galaxyumi pushed a commit to galaxyumi/beast that referenced this pull request Jun 7, 2020
Adjust Av and Age priors, allow verify_params failures

Failures are understood and have to do with the development version of astropy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants