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 new HWO Input Catalog #53

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nwtuchow
Copy link

I've added the HPIC: The Habitable Worlds Preliminary Input Catalog of Tuchow, Stark, and Mamajek (2024) to the data directory and added a function to read it in functions.py. This branch added the functionality to use a new input catalog and does not affect other areas of Bioverse. I've tested the new function and it appears to work properly.

The file size of the HPIC.txt file has been reduced to only include the relevant properties that Bioverse uses. If there are additional stellar properties from the full HPIC catalog (hosted here) which would be useful for Bioverse please let me know.

@matiscke matiscke self-assigned this May 21, 2024
Copy link
Collaborator

@kevinkhu kevinkhu left a comment

Choose a reason for hiding this comment

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

For consistency with the other functions, I recommend changing dist_max to d_max. Also, either change m_V_max to Vmag_max or change the magnitude names in HPIC.txt to m_V, m_J, etc. Otherwise, addition of this function and the HPIC.txt file does not affect the rest of the Bioverse code and can be merged.

@nwtuchow
Copy link
Author

nwtuchow commented May 21, 2024

If I make these changes to my HPIC_integration branch, will it automatically update it in this pull request, or would I need to make a new request?

Now is more consistent with the names of properties in bioverse
@matiscke
Copy link
Collaborator

@nwtuchow, if you just push a new commit to your fork nwtuchow:HPIC_integration, the new commit should automatically show up here and become part of this PR.

@nwtuchow
Copy link
Author

Yes, I made a change on my end earlier, and I saw that it updated the pull request.

Copy link
Collaborator

@matiscke matiscke left a comment

Choose a reason for hiding this comment

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

I reviewed all changes between master and nwtuchow:HPIC_integration and all looks good to me.

However, I don't have the bandwidth to actually test the new functionality right now - sorry. @kevinkhu were you able to run a quick test? Otherwise I can try next week.

I agree that there is little chance that merging this would break Bioverse. Still, before merging I think we should at a minimum:

  • make a generator that includes the new HPIC function
  • actually generate the planets
  • see if the resulting Table object contains what is expected.

In any case, great work on this new addition to Bioverse!!

bioverse/Data/HPIC.txt Show resolved Hide resolved
bioverse/functions.py Show resolved Hide resolved
@matiscke
Copy link
Collaborator

matiscke commented Jun 7, 2024

@nwtuchow Where would be a good place(s) in the documentation to mention the possibility of planet generation with HPIC?

@nwtuchow
Copy link
Author

I think it might make sense to discuss it in the "Generating Planetary systems" section of the documentation. I think it would be also be helpful to describe the different options for the generator that are included with bioverse, such as the different functions for planet occurrence rates, generating host stars . The read_HPIC function could be described alongside the other functions

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