search

CIC-methods / FID-A

Toolbox for simulation and processing of in-vivo magnetic resonance spectroscopy (MRS) data
BSD 3-Clause "New" or "Revised" License
80 stars 72 forks source link

MRSI processing package #36

Closed brendo-k closed 3 years ago

brendo-k commented 3 years ago

Created MRSI processing package and built MRSI trajectory simulation

gdevenyi commented 3 years ago

Hi @brendo-k can you rebase your branch on the latest master HEAD so that the commit history is clean?

brendo-k commented 3 years ago

Hi @gdevenyi did I do the rebase correctly? I also deleted the backup files in the simulation file.

jamienear commented 3 years ago

Thanks @brendo-k! It looks good to my untrained eye. I've attached a network graph showing that your branch seems to be rebased from the most recent commit on the master (07628be), so it seems right. But I will wait to hear from @gdevenyi before I merge, in case there is something I'm missing.

-Jamie

Screen Shot 2020-11-11 at 13 59 23

gdevenyi commented 3 years ago

You can see in the commit history of this PR a large number of entries which are not authored by you but instead merges on master.

Please take a look at https://stackoverflow.com/questions/56708751/github-pull-requests-showing-more-and-more-old-merges

brendo-k commented 3 years ago

Could a reason for the large number of commits be because this branch is behind master? I rebased my code to master and the made a PR to this branch.

gdevenyi commented 3 years ago

Could a reason for the large number of commits be because this branch is behind master?

That is what rebasing is supposed to fix.

brendo-k commented 3 years ago

I'm a little confused. Can you help me figure out where I went wrong?

  1. I made a new branch on my forked repository
  2. I used git rebase upstream/master on the new branch where upstream is the name of this remote repository.
  3. I made a pull request from my branch to the csi_mod branch (which is 48 commits behind master)
gdevenyi commented 3 years ago

Okay, sorry, I see the issue here. This PR is against FID-A/csi_mod, which is very very behind master. Your branch is based off master, so its pulling in all the updated commits.

Is that intentional, or should this PR be against master?

brendo-k commented 3 years ago

Hi Gabe,

No worries thanks for clearing it up! Yes, this is intentional, we didn't want to push it to master since this feature is still unfinished.

jamienear commented 3 years ago

Sorry, I also didn't realize that this pull request was against the csi_mod branch. Maybe I should rebase csi_mod to master before pulling in Brenden's changes? This way it will no longer appear that I am pulling in so many updated commits.

gdevenyi commented 3 years ago

@jamienear that would clear things up slightly, but its up to you :)

jamienear commented 3 years ago

I just re-based the csi_mod branch to master. @gdevenyi do you think I can safely merge the pull request now?

jamienear commented 3 years ago
Screen Shot 2020-11-12 at 13 12 05
gdevenyi commented 3 years ago

Looks good :+1: