Skip to content

Add Mass Flux #362

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

Merged
merged 2 commits into from
Jan 13, 2018
Merged

Add Mass Flux #362

merged 2 commits into from
Jan 13, 2018

Conversation

gojanpaolo
Copy link
Contributor

Fixes #358

Add Mass Flux unit definition
Also added operators with Mass Flux' relation to Density, Speed, Area, and Mass Flow.

Add Mass Flux unit definition
Also added operators with Mass Flux' relation to Density, Speed, Area, and Mass Flow.
Copy link
Contributor

@eriove eriove left a comment

Choose a reason for hiding this comment

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

Some small changes needed here too. Thanks for the pull requests!

"Localization": [
{
"Culture": "en-US",
"Abbreviations": [ "g/s-m²" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the minus sign got misplaced here. Try changing it to g·s⁻¹·m²

I just discovered that http://graphemica.com is a great site for these signs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean g·s⁻¹·m⁻² ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks. I will cross check the numbers once more. This is a sign that that's all the reviewing I should be doing tonight :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot that there are no numbers in the tests when there aren't any units with different bases (imperial, metric etc). I'm done with the review then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eriove Is the naming ok?
GramPerSecondPerSquareMeter
notice two "Per".

This is because MassFlux is defined as MassFlow per Area (MassFlowPerArea).
Then MassFlow is Mass per Time (MassPerTime).
Thus giving us MassPerTimePerArea.

Though I'm not sure if there is a convention to be consistent on other units when dealing with this.

Copy link
Owner

@angularsen angularsen left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@angularsen
Copy link
Owner

Thanks, good work on adding the extra test cases.

@angularsen angularsen merged commit 7d53a69 into angularsen:master Jan 13, 2018
@gojanpaolo
Copy link
Contributor Author

Thank you too!

@gojanpaolo gojanpaolo deleted the AddMassFlux branch January 14, 2018 04:08
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