search

icecube / pisa

Monte Carlo-based data analysis
http://icecube.github.io/pisa/
Apache License 2.0
19 stars 47 forks source link

Osc next2 #712

Closed philippeller closed 3 weeks ago

philippeller commented 1 year ago

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.

atrettin commented 1 year ago

There are a bunch of very old commits that are long obsolete. Is there a way that we can rebase and only pick the commits that are actually new?

atrettin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

LeanderFischer commented 1 year ago

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 commented 1 month ago

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

LeanderFischer commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Hi. Yes oscNext2 is the branch I use, which is collection of small changes over time. So want to go back into main I suspect (for example the fices to the MCEq stage Tania and I reported on) but probably some don't need to. I haven't tried main recently so not sure how any changes in there will affect my analysis, I'll need to give it a try and look at the git logs.

On Wed, Jun 26, 2024 at 9:06 AM Jan Weldert @.***> wrote:

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.

— Reply to this email directly, view it on GitHub https://github.com/icecube/pisa/pull/712#issuecomment-2190971514, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMVI67U6G7FVBAASTYQSI3ZJJR53AVCNFSM6AAAAABJ3VTTQSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJQHE3TCNJRGQ . You are receiving this because you were mentioned.Message ID: @.***>

atrettin commented 1 month ago

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 commented 3 weeks ago

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.