Repository: physiopy/phys2bids

Issue:
Change floats print format
Currently, any print of a float in a file has a format with many decimals.
It would be better to have a formatted print with max 4 decimals.

This doesn't affect the data, only the information in the json and in the log.

Discussion:
eurunuela: @RayStick I see the PR is still a draft. Let @smoia and I know when this is ready for review! ;)
smoia: Hey Rachael - thanks for the PR.
A couple of notes:
- if you prefer to operate on string formatting, I suggest you to check out [f-strings](https://realpython.com/python-f-strings/). They do what you want to do, only faster and tidier - and in fact, you could skip a variable assignment and use f-strings straight where you want, as the logger is already organised with f-string . However, this solution will force the output to be a string (which is OK for the logger, but not for the json).
- there is [this solution](https://stackoverflow.com/questions/10504697/format-a-float-in-python-with-a-maximum-number-of-decimal-places-and-without-ext) that uses `round` instead. It won't force _all_ of your floats to have four digits - so for instance `0` will become `0.0` instead of `0.0000`. It's also nice because it keeps number as floats instead of strings, and it will make your like with tests easier. As you're already using a variable assignment, you can use this solution too!

For the tests:
- the [linting test](https://travis-ci.org/github/physiopy/phys2bids/jobs/675921532) is failing due to the fact that your code is not respecting flake8 standards. I would suggest you to install flake8, pylint, and if you want [linter-flake8](https://atom.io/packages/linter-flake8) in Atom (you use Atom, right?). Once you correct the format, that test will pass.
- the [unit and integration tests](https://travis-ci.org/github/physiopy/phys2bids/jobs/675921534) fail because some of them become _wrong_ with your PR. You will have to change parts of those tests, accordingly to the solution you decide to adopt. If you use `round`, you will have to round the starting time in the tests before any assertion.
smoia: @RayStick , this should be solved and merged in max 2 weeks. Will you be able to meet the deadline (giving us three or four days to review, it means 6th of June)?
RayStick: Big apologies for lack of communication on this. Unfortunately I haven't found as much time as I thought I would to learn Python!

I have implemented the f-string method for the outputs to terminal, and the round method when outputting the json file. Is this better now? If so, I will have a go at the tests. 

I started using Spyder which I think has checked for those standards you mentioned. However I am going to double check.  
RayStick: Still working on the linting test. I installed all the things you suggested and it doesn't seem to think I have any errors. I understand the error (_phys2bids/phys2bids.py:73:21: E225 missing whitespace around operator_) but can't see what issue anywhere. Is 73 the line number?
smoia: It seems to me (and to Travis) that your linting is fine, where do you see the error?
In any case, I think so. _phys2bids/phys2bids.py:73:21_ should mean that column 21, line 73 of the file phys2bids/phys2bids.py has the error reported.
smoia: @RayStick , I think you're seeing it in an old report (Job 718, probably?).
Every time that you push to your branch, the CIs start a new job - the last job in Travis is [858](https://travis-ci.org/github/physiopy/phys2bids/builds/693520410) and the last job in Azure is 20200601 (but Azure shouldn't have any linting test, am I right @rmarkello?).
Always check the details of the jobs at the end of the page!
RayStick: @smoia Oh I see what you mean now, thank you for clarifying. 
rmarkello: Hi all! 

To answer your question @smoia: no, Azure doesn't lint, so it shouldn't be failing for anything related to that.

A slightly larger issue is that the git history here seems to be a bit out-of-sync with the master branch. There are a lot of commits included in the git history of this PR that shouldn't be, making it a bit difficult to review what @RayStick has changed versus what other contributors have changed!

@RayStick, it looks like there are ~two commits that you made in this branch: 9126b5f and 
b76ce20. Can you confirm that's correct? If so, we can probably fix this up so the history reflects your actual changes! Once you let me know I'll give you a couple of commands to run to edit the git history (though you'll also have to confirm whether you're using git via the command-line or via a GUI!).
RayStick: @rmarkello yeah, I noticed that too. I think I know why that happened however the simplest way to solve it might be for me to just make a new PR, with a new branch, because then I can make sure the commit history is not this messy. Do you think that's a good approach? Yes, it was 2 commits and I interact with git via the command-line. 
smoia: @RayStick you can also just force push another clean local branch (instead of opening a new PR), but @rmarkello I'd really like to know why this happens and more importantly how to fix it, because it's not the first time!
RayStick: I am not fully sure what "force push another clean local branch" means but would like to learn! 
And yes, I would like to understand why this sometimes happens too. I _think_ I just pulled from the upstream master to my local branch 'issue26' (to make sure there'd be no merge conflicts and I was up to date with the master) and then pushed to my remote branch 'issue26.'
rmarkello: @RayStick: there are lots of ways to fix this in practice that can get quite complicated depending on how messy things are, but since you only made the two commits I think the easiest way is going to be by: (1) creating a new temporary branch, (2) [cherry-picking](https://stackoverflow.com/questions/9339429/what-does-cherry-picking-a-commit-with-git-mean) the two commits you made, (3) deleting your old `issue26` branch, (4) re-naming your temporary branch to `issue26`, and (5) force pushing the "new" `issue26` branch with "clean" history up to GitHub.

That's a lot of git-specific terminology, so I'll try and break it down into the commands you need to run and what each is doing. Apologies if this is too much detail! I'd prefer to be overly explanatory here since this is a super confusing situation that happens _all the time_ so it's definitely good to know :grin:

### Step one

```
git checkout master
git pull upstream master
git checkout -b tmp
```

Here, our goal is to create a new "clean" temporary branch. We want the base of this branch to be the `master` branch of `phys2bids`, so first we checkout to the `master` branch of your local repository, sync it with the `master` branch of the `upstream` remote, and then create a new `tmp` branch starting at that point in history.

This assumes you have https://github.com/physiopy/phys2bids set up as `upstream` in your local git repository, but from your last comment it sounds like you do. If you don't, replace `upstream` with whatever you have named the `physiopy/phys2bids` remote on your local git repository. You can check what yours is by typing `git remote -v`, the output of which should look something like:

```
origin	https://github.com/raystick/phys2bids (fetch)
origin	https://github.com/raystick/phys2bids (push)
upstream	https://github.com/physiopy/phys2bids (fetch)
upstream	https://github.com/physiopy/phys2bids (push)
```

### Step two

```
git cherry-pick 9126b5f b76ce20
```

Cherry-picking says, as its name implies, "take these commits from my other branch and put them on this branch." Because git hash IDs are unique within a repository, you don't have to specify which branch you're cherry-picking from—it just checks [_all the branches_](https://i.kym-cdn.com/photos/images/original/000/140/938/responsibility12(alternate).png)! 

So this command will basically "replay" your commits (9126b5f and b76ce20) from your `issue26` branch on your `tmp` branch. Cherry-picking is _very_ convenient when you've only made a few changes but they somehow got tangled up in a veritable mess of history! 

We now have a "clean" `tmp` branch with just your changes.

### Step three

```
git branch -D issue26
git branch -m issue26 
```

The first command here deletes the old `issue26` branch (`-D` is for "delete"), and then the second command renames `tmp` to `issue26` (`-m` is for "move"). 

### Step four

```
git push --force --set-upstream origin issue26
```

This will push the new `issue26` branch, with just your two new commits, up to origin. We need to specify `--force` because the history of your local branch and the remote branch (on GitHub) aren't in-sync, but we want our local history (that we just fixed) to take precedence!

The end!

---

I'm honestly not _entirely_ sure why this happened—though if you let me know the exact commands you ran I can try and figure it out! 

Nonetheless, to answer your / @smoia's question, I think the best collaborative git workflow (and indeed, the workflow I use) is the following:

First, you want to make a new branch, make some changes, and push it to GitHub and open a PR:

```
git checkout -b my_new_feature_branch
<make some changes>
git add --all
git commit -m 'I made some changes'
git push -u origin my_new_feature_branch
```

Then, when you realize that your branch is (inevitably) out-of-sync with the master branch on GitHub and you need to update it, you do the following:

1. Swtich to the `my_new_feature_branch` (if you're not already there) with: `git checkout my_new_feature_branch`
2. Fetch the latest changes from GitHub (again, assuming `upstream` is the correct remote name): `git fetch upstream`
3. Make a temporary branch in case something goes awry—this is _critical_, because things _very often go awry_—with:`git branch tmp`
4. Merge the changes from `upstream/master` into your `my_new_feature_branch` with: `git merge upstream/master`
5. Push the merged changes up to GitHub: `git push`
6. Delete your temporary branch, assuming everything worked as expected, with: `git branch -D tmp`

For a *really* clean git history, you can replace `merge` in the last command with [`rebase`](https://stackoverflow.com/questions/804115/when-do-you-use-git-rebase-instead-of-git-merge). Merging will create a merge commit (which will have a commit message like `Merge branch ### from https://github.com/physiopy/phys2bids into ###`), whereas re-basing will avoid this. Instead, rebase completely changes the history of your branch, pretending that all your commits happened _after_ the new commits in `master`. As such, if you use `rebase` you will need to use `git push --force` in step (5) above (because your local git branch history will no longer match the branch history on github!).

(I generally recommend just doing `git merge` because rebasing can get quite messy, quite quickly—though, to be fair, merging can as well! 🤦)

Regardless of whether you use `merge` or `rebase`, if something gets messed up in step (4) and you don't know what happened, you can do the following:

```
git checkout tmp
git branch -D my_new_feature_branch
git branch -m my_new_feature_branch
```

And voila! You're back to where you were right after step (2) and you can try again.

For a pretty good (albeit _slightly_ out-of-date) summary of all of this I would recommend [this page from the nibabel documentation](https://nipy.org/nibabel/gitwash/development_workflow.html).

---

That was a lot of info so _definitely_ let me know if you have any questions! And if you have trouble running the commands to fix up the current PR I'd be happy to hop on a quick video call or something to debug, too!
RayStick: @rmarkello thank you very much for the detailed instructions! Everything made sense as I read through it. I will carry out these steps today and let you know if I come across any issues. 
RayStick: @rmarkello yay, it worked! Thanks again for your clear instructions, they were very easy to implement. 
RayStick: Ok, so the next steps is for me to work out why travis CI build failed. Looking at the errors, I think I can see why, and what I would need to edit. 

However, I am unsure on the workflow of how I go about changing this. Do I just edit the appropriate files in the 'test' directory, then push this change to this remote branch and see if the test passes now? Or how do I do that locally first?
smoia: If you want to check quickly, you can invoke pytest (provided that you installed it!) on the test you're changing locally.
For instance, in a terminal you can navigate to the test folder, then call just one test that is failing (e.g. `test_integration_tutorial` in `test_integration.py`), or a whole group of tests (`test_integration.py`), or even all the tests:
```
cd phys2bids/tests
# Run only one test with a lot of verbosity
pytest test_integration.py::test_integration_tutorial -vv
# Run a full group of tests
pytest test_integration.py
# Run all the tests in the folder
pytest
```
If you run tests locally, pay attention if you add and commit changes to the whole folder (since you would alsotrack the files resulting from running the tests). Just delete the file you didn't have before in the `test/data` folder!
RayStick: Thanks @smoia. I will follow these steps tomorrow morning. 

Patch:
diff --git a/phys2bids/phys2bids.py b/phys2bids/phys2bids.py
index 138a512..593b0e4 100644
--- a/phys2bids/phys2bids.py
+++ b/phys2bids/phys2bids.py
@@ -76,7 +76,7 @@ def print_summary(filename, ntp_expected, ntp_found, samp_freq, time_offset, out
                f'Timepoints expected: {ntp_expected}\n'
                f'Timepoints found:    {ntp_found}\n'
                f'Sampling Frequency:  {samp_freq} Hz\n'
-               f'Sampling started at: {start_time} s\n'
+               f'Sampling started at: {start_time:.4f} s\n'
                f'Tip: Time 0 is the time of first trigger\n'
                f'------------------------------------------------\n')
     LGR.info(summary)
@@ -106,7 +106,7 @@ def print_json(outfile, samp_freq, time_offset, ch_name):
     """
     start_time = -time_offset
     summary = dict(SamplingFrequency=samp_freq,
-                   StartTime=start_time,
+                   StartTime=round(start_time, 4),
                    Columns=ch_name)
     utils.writejson(outfile, summary, indent=4, sort_keys=False)
 
diff --git a/phys2bids/physio_obj.py b/phys2bids/physio_obj.py
index 1dfe3c3..3eb1ce1 100644
--- a/phys2bids/physio_obj.py
+++ b/phys2bids/physio_obj.py
@@ -459,9 +459,9 @@ class BlueprintInput():
                                     lambda x: x != 0) if is_true])
         if flag == 1:
             LGR.info(f'The number of timepoints according to the std_thr method '
-                     f'is {num_timepoints_found}. The computed threshold is {thr}')
+                     f'is {num_timepoints_found}. The computed threshold is {thr:.4f}')
         else:
-            LGR.info(f'The number of timepoints found with the manual threshold of {thr} '
+            LGR.info(f'The number of timepoints found with the manual threshold of {thr:.4f} '
                      f'is {num_timepoints_found}')
         time_offset = self.timeseries[0][timepoints.argmax()]
 


Test patch:
diff --git a/phys2bids/tests/test_integration.py b/phys2bids/tests/test_integration.py
index 232e8a0..1ce893b 100644
--- a/phys2bids/tests/test_integration.py
+++ b/phys2bids/tests/test_integration.py
@@ -82,7 +82,7 @@ def test_integration_tutorial():
     # Check sampling frequency
     assert check_string(log_info, 'Sampling Frequency', '1000.0')
     # Check sampling frequency
-    assert check_string(log_info, 'Sampling started', '0.24499999999989086')
+    assert check_string(log_info, 'Sampling started', '0.2450')
     # Check start time
     assert check_string(log_info, 'first trigger', 'Time 0', is_num=False)
 
@@ -92,7 +92,7 @@ def test_integration_tutorial():
 
     # Compares values in json file with ground truth
     assert math.isclose(json_data['SamplingFrequency'], 1000.0)
-    assert math.isclose(json_data['StartTime'], 0.245)
+    assert math.isclose(json_data['StartTime'], 0.2450)
     assert json_data['Columns'] == ['time', 'Trigger', 'CO2', 'O2', 'Pulse']
 
     # Remove generated files
@@ -126,7 +126,7 @@ def test_integration_acq(samefreq_full_acq_file):
     # Check sampling frequency
     assert check_string(log_info, 'Sampling Frequency', '10000.0')
     # Check sampling started
-    assert check_string(log_info, 'Sampling started', '10.425107798467103')
+    assert check_string(log_info, 'Sampling started', '10.4251')
     # Check start time
     assert check_string(log_info, 'first trigger', 'Time 0', is_num=False)
 
@@ -136,7 +136,7 @@ def test_integration_acq(samefreq_full_acq_file):
 
     # Compares values in json file with ground truth
     assert math.isclose(json_data['SamplingFrequency'], 10000.0)
-    assert math.isclose(json_data['StartTime'], 10.425107798467103)
+    assert math.isclose(json_data['StartTime'], 10.4251)
     assert json_data['Columns'] == ['time', 'RESP - RSP100C', 'PULSE - Custom, DA100C',
                                     'MR TRIGGER - Custom, HLT100C - A 5', 'PPG100C', 'CO2', 'O2']
 
@@ -181,7 +181,7 @@ def test_integration_multifreq(multifreq_acq_file):
     # Check sampling frequency
     assert check_string(log_info, 'Sampling Frequency', '625.0')
     # Check sampling frequency
-    assert check_string(log_info, 'Sampling started', '0.29052734375')
+    assert check_string(log_info, 'Sampling started', '0.2905')
     # Check start time
     assert check_string(log_info, 'first trigger', 'Time 0', is_num=False)
 
@@ -191,7 +191,7 @@ def test_integration_multifreq(multifreq_acq_file):
 
     # Compares values in json file with ground truth
     assert math.isclose(json_data['SamplingFrequency'], 625.0)
-    assert math.isclose(json_data['StartTime'], 0.29052734375)
+    assert math.isclose(json_data['StartTime'], 0.2905)
     assert json_data['Columns'] == ['PULSE - Custom, DA100C']
 
     """
@@ -208,7 +208,7 @@ def test_integration_multifreq(multifreq_acq_file):
     # Check sampling frequency
     assert check_string(log_info, 'Sampling Frequency', '10000.0')
     # Check sampling started
-    assert check_string(log_info, 'Sampling started', '10.425107798467103')
+    assert check_string(log_info, 'Sampling started', '10.4251')
     # Check start time
     assert check_string(log_info, 'first trigger', 'Time 0', is_num=False)
 
@@ -218,7 +218,7 @@ def test_integration_multifreq(multifreq_acq_file):
 
     # Compares values in json file with ground truth
     assert math.isclose(json_data['SamplingFrequency'], 10000.0)
-    assert math.isclose(json_data['StartTime'], 10.425107798467103)
+    assert math.isclose(json_data['StartTime'], 10.4251)
     assert json_data['Columns'] == ['time', 'RESP - RSP100C',
                                     'MR TRIGGER - Custom, HLT100C - A 5', 'PPG100C', 'CO2', 'O2']
 
@@ -266,7 +266,7 @@ def test_integration_heuristic():
     # Check sampling frequency
     assert check_string(log_info, 'Sampling Frequency', '1000.0')
     # Check sampling started
-    assert check_string(log_info, 'Sampling started', '0.24499999999989086')
+    assert check_string(log_info, 'Sampling started', '0.2450')
     # Check start time
     assert check_string(log_info, 'first trigger', 'Time 0', is_num=False)
 
@@ -277,7 +277,7 @@ def test_integration_heuristic():
 
     # Compares values in json file with ground truth
     assert math.isclose(json_data['SamplingFrequency'], 1000.0)
-    assert math.isclose(json_data['StartTime'], 0.24499999999989086)
+    assert math.isclose(json_data['StartTime'], 0.2450)
     assert json_data['Columns'] == ['time', 'Trigger', 'CO2', 'O2', 'Pulse']
 
     # Remove generated files