3: Peer review of fit results
Once the initial fitting process has been completed, the next step of the procedure is peer review. This is an iterative process between the fitter and an independent reviewer. The following outlines the steps to be undertaken during peer review and each will be described in more detail below. In what follows F describes the fitter's responsibilities and R those of the reviewer.
- R: Add yourself as a reviewer
- R: Make sure you have the main repo added as the remote upstream
- R: Make sure you are located on the main branch of your forked repo
- R: Pull any changes from the upstream repo
- R: Create a local branch and merge the changes
- R: Check the fits and document any changes
- R: Add and commit any suggested changes
- R: Push changes to fitter's branch
- F&R: Iterate via comments and converge to final solution
- R: Approve final solution
- F/R: Merge into main branch
Note that all pull requests have to undergo a review by a researcher other than the person who created the PR. This also applies to the initial fitting. A quick look at the PRs listed in the main repo will show that a review is required.
To get started, the first thing you want to do is assign yourself to a PR. This can be done on the right-hand side of the PR screen...
navigate to "reviewer" and add yourself to the PR.
The first thing you may notice is that there may be conflicts in the run_scouse_chunks.py script. Click on the "resolve conflicts" button that will take you to the next page.
The conflicting lines are highlighted. You can simply delete the conflicting lines. Do do this, cross-reference the chunkID with that of the PR. In the example above, the PR refers to chunk 20. So we can simply delete the lines referring in this case to chunk 21 as so...
Note how the highlighting has now been removed and the "Mark as resolved" button is now clickable. Clicking on this will produce a green button "Merge". Clicking this will return you to the PR which should now look like this...
Note that this is the beginning of the review process. Do not click on the "Merge pull request" green button. Instead continue with the instructions below to continue the review process from the command line.
The first thing that we want to do is make sure that the main repository is added and linked to your fork. If you followed the instructions outlined in page 1 of this wiki, then this should already be the case. To double check first type...
git remote -v
if everything has been set up correctly then you should see something like...
origin https://github.com/jdhenshaw/WP2_pilot.git (fetch)
origin https://github.com/jdhenshaw/WP2_pilot.git (push)
upstream https://github.com/ACES-CMZ/WP2_pilot.git (fetch)
upstream https://github.com/ACES-CMZ/WP2_pilot.git (push)
If the upstream isn't there you will need to type...
git remote add upstream https://github.com/ACES-CMZ/WP2_pilot.git
You can type git remote -v again to ensure this now exists. Note that you can skip this step if the remote upstream was there to start with.
This should be self explanatory but to begin, make sure you are on the main branch by typing...
git checkout main
Here we want to pull the latest version of the main branch to our local branch. This can be done by typing...
git pull upstream main
This next step of the process follows the first few command line instructions outlined in GitHub for merging a pull request. Though note that we potentially want to make modifications so we don't want to follow these instructions through to completion. We start out by creating a local branch and merging the changes from the PR branch into here for inspection...
git checkout -b s1.[chunkID].scousepy main
The next step will look slightly different depending on how the fitting was performed. If the fitting was run prior to forking of the main branch, you should run the following...
git merge origin/s1.[chunkID].scousepy
If this does not work (error msg: "merge: origin/s1.22.scousepy - not something we can merge"), you will need to pull the changes from the fitters remote repository, which will look something like...
git pull https://github.com/USERNAME/WP2_pilot.git s1.[chunkID].scousepy
for example...
git pull https://github.com/aschmiedeke/WP2_pilot.git s1.22.scousepy
where the chunkID is the chunk we are wanting to check that has been fitted by someone else in the team.
To begin the checking procedure, we first need to modify the run_scouse_chunks.py script. Namely you will need to update the following lines
s1file='s1.[chunkID].scousepy' # You will need to update these accordingly!!
s2file='s2.[chunkID].scousepy' # You will need to update these accordingly!!
Secondly, you will need to modify the stage 2 command setting refit=True. This will allow you to re-enter the fitter, but this time all of the model solutions that are displayed are those selected by the other user (as opposed to ScousePy's initial guesses based on the SNR and alpha parameters...
s = scouse.stage_2(config=config_file, refit=True, s1file=s1file, s2file=s2file)
At this point you will need to check through the fits. This process should be much quicker than making the fits yourself. If you disagree with any of the users fits, or a better solution can be found, the fits can be modified. Changing the fits will automatically update the s2.[chunkID].scousepy file as well as the output file produced on completion.
Should you wish to make changes to any of the fits, make sure that you take before and after screenshots for documentation and discussion with the fitter. The fits can be changed directly, but the changes should be documented via the comments in the relevant PR. Please also ensure that the spectrum index is either visible in the screenshot. Try to list all changes/screenshots into a single comment such that the original fitter can simply quote reply (and to not fill their email inbox).
Once the fit check is complete add your changes and commit them using...
git add -A
git commit -v -m "[commit statement"]
Be sure to tag the relevant PR index in your commit statement.
Now we want to push the changes. To do this use the following...
git push <remote repo> <your branch name>:<original branch name>
An example of this would be the following...
git push https://github.com/username-fitter/WP2_pilot.git s1.[chunkID].scousepy:s1.[chunkID].scousepy
This will push the changes to the remote repo and update the PR. At which point further review will be necessary.
Once the reviewer has pushed their suggested changes, now the iterative process begins towards convergence. Using the documented suggested changes in the PR comments, the fitter should check each suggested change and either approve it or open a dialogue with the reviewer. For example...like done here...(see the original PR conversation here:
At this point, there may be instances where the fitter and the reviewer are in total agreement. If this is so, the suggested changes from the reviewer can be approved and you can move on to the next stage.
However, there may be other instances where individual fits may require a different solution that is agreed upon by the fitter and the reviewer. To reach an agreement, both parties will need to ensure they have the latest version of the stage 2 chunk pulled from the fitter's repo. The key point here is to reach a a mutual agreement before any further commits are made. This discussion should be documented in the PR comments. Once an agreement has been reached, either can implement the agreed solution and then commit the change. In the unlikely event that an agreement cannot be reached, get help from someone else in the team.
Once all changes have been made, committed and pushed to the fitter's branch, the reviewer then is required to click on the add your review link shown below...
Next click green review changes box in the top right corner, add a comment and select approve changes like so...
The final part of the process is to then merge the branch into main. This can be done by clicking the big green merge branch button on the PR page.
In short, the above process will look something like the following...
-
git remote add upstream [email protected]:ACES-CMZ/WP2_pilot.git(only need to do this command once.)- If you do
git remote -v, you should seeoriginpointing to your name andupstreampointing toACES-CMZ.
- If you do
git checkout maingit pull upstream main- Do “Step 1” in the github command line instructions; they may look like this:
git checkout -b s1.INDEX.scousepy maingit merge origin/s1.INDEX.scousepy
- Modify run_scouse_chunks.py and check fits
git add allgit commit -m “fitting checked and completed #PR-INDEX”-
git push <remote repo> <your branch name>:<original branch name>- Example:
git push https://github.com/username-fitter/WP2_pilot.git s1.INDEX.scousepy:s1.INDEX.scousepy
- Example: