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

Dev #29

Closed
wants to merge 11 commits into from
Closed

Dev #29

wants to merge 11 commits into from

Conversation

RyotaInagaki1
Copy link

A draft of the BPASS evolutionary model class.
Test isochrone files are in the /g/lu/models/evolution/BPASS/v2.2/iso/z020 directory. Note that the naming convention for the isochrone files is slightly different that it would be for other models and I have at least attempted to account for that in the BPASS object's methods.

Copy link
Contributor

@jluastro jluastro left a comment

Choose a reason for hiding this comment

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

Overall, a good start! Code needs to be cleaned up just a bit to follow PEP 8 throughout and to make it easier to read. Be sure to add comments/documentation for every class/method.

Primary issues, other than formatting, is handling the location of the evolution models.

@@ -1615,6 +1615,91 @@ def get_orig_pisa_isochrones(metallicity=0.015):

return data


Copy link
Contributor

Choose a reason for hiding this comment

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

Please follow Python PEP 8 style guide throughout.

self.age_list=[round(6.0+x*0.1,1) for x in range(0, 51)]
#Possible metallicities for BPASS models
self.z_list=[10**-5, 10**-4,10**-3, 0.002, 0.003, 0.004, 0.006, 0.008, 0.010, 0.014, 0.020, 0.030, 0.040]
self.models_dir='/g/lu/models/evolution/BPASS/v2.2/'
Copy link
Contributor

Choose a reason for hiding this comment

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

The directory shouldn't be hard-coded... use the environment variable as is done in the other StellarEvolution objects.

self.z_list=[10**-5, 10**-4,10**-3, 0.002, 0.003, 0.004, 0.006, 0.008, 0.010, 0.014, 0.020, 0.030, 0.040]
self.models_dir='/g/lu/models/evolution/BPASS/v2.2/'
#Possible masses
self.mass_list=[round(0.1, 1)]+[round(0.12+x*0.02, 2) for x in range(95)]+[round(2.05+x*0.05, 2) for x in range(20)]+[round(3.1+0.1*x,1) for x in range(70)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please split into for loops where you append to the mass_list. This is too convoluted to read right now.

self.mass_list=self.mass_list+[11+x for x in range(90)]+[125+25*x for x in range(8)]+[400]+[500]
self.z_solar=0.02
self.z_file_map={10**-5: 'zem5/', 10**-4: 'zem4/', 0.003:'z003/',0.002: 'z002/', 0.003: 'z003/', 0.004: 'z004/', 0.006:
'z006/',0.008: 'z008/', 0.010:'z010/',0.014:'z014/', 0.020: 'z020/', 0.030: 'z030/',0.040:'z040/' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Include a return and space between methods.

self.z_solar=0.02
self.z_file_map={10**-5: 'zem5/', 10**-4: 'zem4/', 0.003:'z003/',0.002: 'z002/', 0.003: 'z003/', 0.004: 'z004/', 0.006:
'z006/',0.008: 'z008/', 0.010:'z010/',0.014:'z014/', 0.020: 'z020/', 0.030: 'z030/',0.040:'z040/' }
def isochrone(self, filePath="", age=1*10**8, metallicity=0.0, binary=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need a filePath... lets hardcode to a specific sub-directory structure under the environment variable specified.

currentMass=Column([0.0]*len(iso), name='mass_current')
colZ=Column([self.z_list[zindex]]*len(iso), name='Z')
colP=Column([-1]*len(iso), name='phase')
iso.add_column(colG)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest going ahead and merging the add_column and Column() decorations for each variable.

@jluastro
Copy link
Contributor

You should also add tests for this new class to ensure things are returned properly.

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.

2 participants