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

Osc next2 #712

Closed
wants to merge 148 commits into from
Closed

Osc next2 #712

wants to merge 148 commits into from

Conversation

philippeller
Copy link
Collaborator

Hey All,

This will be major work to make this PR, but this is absolutely critical!

Please, everyone who worked on this branch start looking into this.

bourdeet and others added 30 commits February 24, 2020 14:24
@atrettin
Copy link
Contributor

atrettin commented Dec 6, 2022

Is @ts4051 the only one who can make changes to this branch? Because if so, this will severely complicate getting this done. Perhaps it would be better if we copied all of this into a non-master branch in the main repo first, so that we all have writing access to it, and then we make a PR from that feature branch. Then we can all work on it together without having to go through Tom's fork every time.
EDIT: Nevermind, it looks like we can all write there.

@atrettin
Copy link
Contributor

atrettin commented Dec 6, 2022

I found that a lot of commits already exist in old pull requests, but they are not recognized as identical because they were squashed before they were merged. For example, a lot of Étiennes old commits are in PR #639 . Is there an automated way that we can search for changes that we already have and squash those away?

@philippeller
Copy link
Collaborator Author

I think some rebase magic may get rid of the older, redundant commits that are already in main?

Copy link
Contributor

@shiqiyugit shiqiyugit left a comment

Choose a reason for hiding this comment

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

new code for bin masking. don't see conflicts or other changes from the folk.

Copy link
Contributor

@shiqiyugit shiqiyugit left a comment

Choose a reason for hiding this comment

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

new code for bin masking. don't see conflicts or other changes from the folk.

@philippeller philippeller marked this pull request as draft May 4, 2023 13:00
@LeanderFischer
Copy link
Collaborator

What is this status of this, actually? Could we get this out of the way, as well? (assigned a bunch more people to get eyes on this)

@philippeller
Copy link
Collaborator Author

I suppose this is again hopelessly out-of-date? Should we just close this PR?

@LeanderFischer
Copy link
Collaborator

I suppose this is again hopelessly out-of-date? Should we just close this PR?

Honestly, I don't know who's even using this branch, I am certainly now and I ran a fit using PISA main and the flercnn sample, so I don't know why people would need anything else. Maybe raise this at a le-osc call and then decide?

@JanWeldert
Copy link
Contributor

I guess Tom might use this branch. We should at least ask him if there are any functionalities missing in main. Maybe adding one or two things is sufficient and way more easy than trying to merge the whole thing.

@ts4051
Copy link
Contributor

ts4051 commented Jun 27, 2024 via email

@atrettin
Copy link
Contributor

The general problem with using a separate branch like this for analysis work is that it circumvents the testing and review processes that have been set up on the master branch. This has caused errors to make it into analyses in the past that would have been avoided if the review process had been followed. Furthermore, this makes it harder to later merge individual features into the master branch. For instance, fixes to the MCEq stage should have had a separate branch for that specific feature and a Pull Request for just that feature. Instead, if we want these fixes, we need to review all of the other unrelated changes as well before we can merge.
I understand that it's nice to have a "personal" branch where you can just make changes without asking anyone, but then it can't be used to commit important features that are of general interest for the rest of the working group.

@LeanderFischer LeanderFischer removed this from the PISA 4.2 milestone Jul 26, 2024
@LeanderFischer
Copy link
Collaborator

I don't see this happening anytime soon, and everyone is using the master branch for their analyses (as they should), so I'm closing this. @ts4051, it's up to you to add changes that you need for your analysis into master, but please do that bit/feature wise and not in a massive single PR.

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.