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

New functions molar_density and mass_density added #30

Merged
merged 10 commits into from
Aug 2, 2021

Conversation

rameshputalapattu
Copy link
Contributor

@rameshputalapattu rameshputalapattu commented Aug 2, 2021

Hi @pw0908,
I have added new functions molar_density and mass_density and the corresponding unitful versions as well. I found that having functions which calculate densities to be quite handy based on my experience at work using other GERG2008 (fortran and C++) implementations. I have added a self contained test set for these functions.
Please see if it fits into the upstream code base and can be merged. I will be glad if these changes could get in.
Please let me know if you have any comments.

Regards
Ramesh

@pw0908
Copy link
Member

pw0908 commented Aug 2, 2021

Hi Ramesh,

Thanks a lot for this! We agree that densities would certainly a useful property to support (we have a long list of properties we want to support and just need to get around to it...).

However, you have also pointed out something we hadn't considered: supporting both mass and molar basis properties. A simple solution could be just to do as you did and simply have molar_property and mass_property functions. We do wonder if there isn't a cleaner way to do this?

One suggestion is something like:
mass_basis(property(model,p,T))
molar_basis(property(model,p,T))
If you have any suggestions, that would be great. I'll leave the pull request open until we've decided what's the best thing to do.

Cheers,

Pierre

@ypaul21
Copy link
Member

ypaul21 commented Aug 2, 2021

We probably need to pass in the model struct directly to the function to allow it to access the molar mass. It would probably have to be something like

mass_basis(property, model, p, T, z)

Not sure if it’s an elegant-enough solution though.

But mass and molar densities are extremely common, and they’re some of the few intensive properties that we could support (the rest are mostly extensive). I think I’m personally fine with accepting this PR for these two properties.

@pw0908
Copy link
Member

pw0908 commented Aug 2, 2021

In that case we'll merge then. I'll move the tests into the main methods_test file later on. We can sort out the molar and mass basis issue later on.

@pw0908 pw0908 merged commit 9ce39b2 into ClapeyronThermo:master Aug 2, 2021
pw0908 added a commit that referenced this pull request Apr 28, 2022
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