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

Adding PARSEC/COLIBRI + MIST stellar model support #60

Merged
merged 4 commits into from
Feb 24, 2017

Conversation

lcjohnso
Copy link
Member

Copy link
Member

@mfouesneau mfouesneau left a comment

Choose a reason for hiding this comment

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

1 bug in the imports: they need to be relative

a few warnings (code and science)

@@ -51,7 +51,7 @@
# 't_isochrones', 't_spectra', 't_priors', 't_seds' ]

def make_iso_table(outname, logtmin=6.0, logtmax=10.13, dlogt=0.05,
z=[0.019], trackVersion=2.3):
z=[0.0152], trackVersion=2.3, oiso=None):
Copy link
Member

Choose a reason for hiding this comment

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

I would add in the doc string that 0.015 comes with the new Zsun adopted in the recent Padova models. Just for completeness with documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

cond = '~((M_ini < 12.) & (stage == 0))' # do not include Pre-MS
t = t.selectWhere('*', cond)
#LCJ WIP: starting to enable
oiso = oiso or isochrone.PadovaWeb()
Copy link
Member

Choose a reason for hiding this comment

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

I found this not to work all the time. Maybe being more explicit?

if oiso is None: 
    oiso = isochrone.PadovaWeb()

Copy link
Member

Choose a reason for hiding this comment

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

I see that all classes derive from Isochrone class. That's good. So maybe my previous comment should even be

if oiso is None: 
    oiso = isochrone.PadovaWeb()
else if not isinstance(oiso, Isochrone):
    raise AttributeError("oiso is not of Isochrone type")

Copy link
Member Author

@lcjohnso lcjohnso Feb 24, 2017

Choose a reason for hiding this comment

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

@mfouesneau -- Can you elaborate on "not working all the time"? I haven't tested this yet due to the need to rebase before fully implementing changes back to datamodel.py. I was following format used for stellib objects in make_spectra(), but happy to make a change if necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I did not find what goes wrong sometimes and not all the time. I know I used it many times and not surprisingly in stellibs. I found safer to any case my alternative even if less compact.

t.header['NAME'] = '{0} Isochrones'.\
format('_'.join(outname.split('_')[:-1]))

# Isochrone filtering, check that no AGB stars are removed
Copy link
Member

Choose a reason for hiding this comment

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

This comes from Yumi's needs. It filters out PMS data points. Maybe moving that back into the PadovaWeb class as an option?

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 moved this cut back into PadovaWeb, in _filter_iso_points(), but have all cuts commented out. I need to enable keyword passing (filterPMS & filterBad boolean variables) to this function so that these cuts can be optionally applied.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just document the filter function and remove the _ in front (that indicates private/don't use by convention). The important part is to tell Yumi too. It makes sense that you don't want it to be generic for all projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think filtering PMS leaves as an option is a good idea.

@@ -16,9 +16,10 @@
from ...external.eztables.table import recfunctions
from ...external.ezunits import unit, hasUnit
from ...config import __ROOT__
from ezpadova import cmd as _cmd
from ezpadova import parsec
Copy link
Member

@mfouesneau mfouesneau Feb 24, 2017

Choose a reason for hiding this comment

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

Need to correct the imports

from .ezpadova import parsec
from .ezmist import mist

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

iso_table.remove_columns(['Age', 'logTe', 'Mini', 'Mass', 'label'])
# Remove age-specific Z, rename Zini as Z
iso_table.remove_columns(['Z'])
iso_table.add_column('Z', iso_table['Zini'][:])
Copy link
Member

Choose a reason for hiding this comment

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

Careful, that new for Padova, they predict the initial and current Z values meaning Z(tage) is variable like the current mass in a consistent way (was not before)
We should document that somewhere that we only give Zini

Copy link
Member Author

Choose a reason for hiding this comment

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

To my knowledge, Z==Zini is assumed throughout the codebase, but we should make that explicit in datamodel.py -- that's on my ToDo.

Copy link
Member

Choose a reason for hiding this comment

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

I do not think it will change a lot in the content but worth mentioning that the reported Z values are Zinit not current Z as given by Padova (I don't know if MESA gives current)

#self.logtmin = logtmin
#self.logtmax = logtmax
#self.dlogt = dlogt
#self.Z = Z
Copy link
Member Author

Choose a reason for hiding this comment

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

@mfouesneau -- Can you comment on this? Acceptable to remove these attributes of the PadovaWeb object?

Copy link
Member

Choose a reason for hiding this comment

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

Don't you need these to query the website?

Copy link
Member Author

Choose a reason for hiding this comment

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

No -- I successfully ran a model generation all the way through to fitting with these lines commented. The values necessary to define the grid are passed as parameters directly to the object's _get_t_isochrones() function within make_iso_table() in models.py, and these are never touched.

Copy link
Member

Choose a reason for hiding this comment

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

Then you're good to me. No need to keep it. They come from the header of the files from the website in case someone needs to access to it somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

To clarify: these object attributes (self.logtmin, self.logtmax, self.dlogt) for the ezIsoch object are used -- perhaps that's what you remembered.

@lcjohnso lcjohnso changed the title [WIP] Adding PARSEC/COLIBRI + MIST stellar model support Adding PARSEC/COLIBRI + MIST stellar model support Feb 24, 2017
@karllark karllark merged commit 42253d0 into BEAST-Fitting:master Feb 24, 2017
@karllark karllark deleted the colibri branch February 24, 2017 23:39
@lcjohnso lcjohnso mentioned this pull request Feb 25, 2017
galaxyumi pushed a commit to galaxyumi/beast that referenced this pull request Jun 7, 2020
Adding PARSEC/COLIBRI + MIST stellar model support
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.

4 participants