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

Add binned stellar mass - BH mass plot #275

Merged
merged 10 commits into from
Mar 24, 2024
Merged

Conversation

FHusko
Copy link
Contributor

@FHusko FHusko commented Mar 20, 2024

No description provided.

Copy link
Collaborator

@EvgeniiChaikin EvgeniiChaikin left a comment

Choose a reason for hiding this comment

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

Thank you for the nice update @FHusko!

Please address a few small comments in the code, and update the version of the observational data submodule. Then we can merge this pull request.

colibre/scripts/bh_accretion_evolution.py Show resolved Hide resolved
colibre/auto_plotter/black_holes.yml Outdated Show resolved Hide resolved
@FHusko
Copy link
Contributor Author

FHusko commented Mar 21, 2024

Thanks for the comments, @EvgeniiChaikin. Could you please remind me how I can update the reference to the observational data? I had forgotten that needs to be done, I thought it automatically points to the current master.

Unrelated to that, I am working to add very soon add high-redshift BH mass - stellar mass data, as well as BH mass functions. Would you prefer to add these (the changes to the pipeline required for those, I mean) as part of this PR, or a separate one?

@EvgeniiChaikin
Copy link
Collaborator

Let's finish this PR because it is almost ready; and for other, larger changes, create a separate PR.

Your repository's observational data points to the master branch that is 2 months old.

To update the submodule: within your branch, go to the observational data submodule, then checkout from the detached state to the master branch, then do git pull on the master branch, and then check out to the latest commit in the master branch. Your pipeline-configs branch, which you are trying to merge here, should pick up that change in the state of the submodule. Just wrap that change into a new commit and push it here.

@FHusko
Copy link
Contributor Author

FHusko commented Mar 22, 2024

Okay, thanks! I think it should now be pointing to the correct version of the submodule.

@EvgeniiChaikin
Copy link
Collaborator

Something went wrong: the submodule has not been updated fully. Its state has changed from being 2 months old to being 2 weeks old, but, for example, there was a comment yesterday to the master branch, which I cannot see in your branch.

Namely, based on your branch, I see
Screenshot from 2024-03-22 13-44-57

Screenshot from 2024-03-22 13-45-09

But the correct recent history of the observational data submodule should be https://github.com/SWIFTSIM/velociraptor-comparison-data/commits/master/

@FHusko
Copy link
Contributor Author

FHusko commented Mar 22, 2024

I hope it is finally matching.

@EvgeniiChaikin
Copy link
Collaborator

Thanks, all looks good!!

@EvgeniiChaikin
Copy link
Collaborator

The last remaining step before merging is to rebase to the latest version of the master branch pipeline-configs.

@FHusko
Copy link
Contributor Author

FHusko commented Mar 23, 2024

When I do this (git rebase master) it says that it is up-to-date.

@EvgeniiChaikin
Copy link
Collaborator

On GitHub, I see that your branch is 3 commits behind.
Screenshot from 2024-03-24 14-30-07

I just noticed that you are merging a branch from a forked repository, instead of a branch from the original repository. This is why it may be more complicated to synchronize with the master branch of the original repository.

One problem with being out of sync, is that even though the submodule in your repo is now correct and up to date, in the file changes of this MR, I still see that the submodule is being updated, which should not be the case.

@FHusko
Copy link
Contributor Author

FHusko commented Mar 24, 2024

Thanks for noticing! Should be in sync now :)

@EvgeniiChaikin EvgeniiChaikin merged commit fef01bc into SWIFTSIM:master Mar 24, 2024
1 check passed
@EvgeniiChaikin
Copy link
Collaborator

Thanks! Now everything is indeed in sync. I've just merged the branch.

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