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

Enh/bids units #224

Merged
merged 4 commits into from
Jun 2, 2020
Merged

Enh/bids units #224

merged 4 commits into from
Jun 2, 2020

Conversation

smoia
Copy link
Member

@smoia smoia commented Jun 2, 2020

Closes nothing, but follows #222 .

The code in master is nice, but there are cases that might not be handled.
For instance, if 'mmHg' (Which is a non-BIDS unit we might end up having, since it's the standard unit for some gas applications e.g. CVR) is passed as an argument to bidsify_units, it would return mmhg. The same can be said for BigV or MMLIE (examples adapted from the test). It's not a huge deal, but if it's not a SI unit, at least we should leave it as it is without changing it.
There's still cases that are not supported (e.g. non-SI term plethora would return 'plethorA'), but that's for another day (and I hope no one would measure "0.56 plethora of physiological activity" in the meantime).

Proposed Changes

  • Little code refactoring to support more cases
  • Add more testing cases, also reorganise weird aliases tests as a dictionary similarly to the current real unit test.
  • Quick dictionary reordering to lower the possibility of clashes
  • Uppercase all the constants

@smoia smoia added the BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) label Jun 2, 2020
@smoia smoia requested a review from vinferrer June 2, 2020 17:02
@codecov
Copy link

codecov bot commented Jun 2, 2020

Codecov Report

Merging #224 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #224   +/-   ##
=======================================
  Coverage   93.66%   93.66%           
=======================================
  Files           8        8           
  Lines         679      679           
=======================================
  Hits          636      636           
  Misses         43       43           
Impacted Files Coverage Δ
phys2bids/bids_units.py 100.00% <100.00%> (ø)

Copy link
Member

@rmarkello rmarkello left a comment

Choose a reason for hiding this comment

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

These small updates all look + seem reasonable to me! Thanks @smoia 👍

@smoia smoia merged commit f294930 into physiopy:master Jun 2, 2020
@smoia smoia deleted the enh/bids-units branch June 13, 2020 22:13
@smoia smoia added the released This issue/pull request has been released. label Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugFIX This PR generally closes a `Bug` issue, and increments the patch version (0.0.+1) released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants