Skip to content

Conversation

russelljahn
Copy link
Contributor

I'm using BPM and heart rate in my own projects, so I figured it would be a good addition.

@angularsen
Copy link
Owner

angularsen commented Feb 22, 2019

Hi and thanks for contributing!

Would it be possible for you to use CyclePerMinute unit instead?

I can understand BPM would be useful for things like Frequency.Parse("120 bpm"), and heartbeat.ToString() to output 120 BPM, but I think this is a slippery slope because I can think of many variations of "per second" or "per minute" counts - such as RPM (revolutions per minute) a RotationalSpeed unit, and more silly examples like popcorn pops per second or cars per hour as a measure of traffic. All of these can be considered frequency measures. Where should we draw the line for what should be added to the library and not?

I'm just not sure. On one hand BPM is widely used and it would let you parse and ToString() the "BPM" unit abbrevation. On the other hand, I don't think it adds much value in terms of unit conversion (what other heat rate units are used?) and we already have the CPM unit. Maybe it's better for you to create your own application specific type HeartRate to represent this quantity and possibly convert to other frequencies? We generally try to keep UnitsNet generic and keep too domain specific units out of it, but this is an edge case I feel.

What do you think @tmilnthorp ?

@tmilnthorp
Copy link
Collaborator

While I agree about things like popcorn pops per second and being careful about what we allow, I think BPM is so common for heartrates and music tempos that it is valid to be included.

@angularsen
Copy link
Owner

Good point about music, didn't think of that usecase. That helps justify this as its own unit of frequency. I can stand behind that 👍

@angularsen angularsen merged commit 1654fa0 into angularsen:master Feb 25, 2019
@angularsen angularsen changed the title Added BeatPerMinute (unit used for measuring heart rate) as a frequency. Add Freqency.BeatPerMinute (#611) Feb 25, 2019
angularsen added a commit that referenced this pull request Feb 25, 2019
@angularsen
Copy link
Owner

Nuget on the way out.
Release UnitsNet/4.11.0 · angularsen/UnitsNet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants