[Feature Request] Make sure calculation works for vector inputs.

banfelder / psychrolib

Fork of psychrometrics/psychrolib to facilitate implementation in R

MIT License

2 stars 0 forks source link

[Feature Request] Make sure calculation works for vector inputs. #1

Open hongyuanjia opened 5 years ago

hongyuanjia commented 5 years ago

Thanks for this pretty neat port of psychrolib.

Current implementation may work fine for single value calculation. However, it would be great to make sure it also works for vector inputs. The efficiency would go extreme low if the user uses loops to get a vector output.

For some functions, replace max() with pmax() should just be enough to let them work for vector inputs.

https://github.com/banfelder/psychrolib/blob/69e25d524fcfb0495cfe2595ba0a9821586a7b4d/src/r/R/saturated_air_calculations.R#L60-L72

But for others, this may involve a little bit more work.

https://github.com/banfelder/psychrolib/blob/69e25d524fcfb0495cfe2595ba0a9821586a7b4d/src/r/R/saturated_air_calculations.R#L14-L48

I can send a pull request if you are interest in this.

banfelder commented 5 years ago

I think this would be a natural extension of the current library, and clearly more efficient than looped calls. Since the original PsychroLib library is scalar only at the moment, I think it would be prudent to see if this R implementation is accepted for a merge before changing the interface. If it is, then I think coordinating vectorized implementations across languages would be important to try to keep the APIs as consistent as possible. The original authors have already proposed this for the Python implemetation, so if that moves forward, I'd want to track that implementation as closely as the different languages allow.

hongyuanjia commented 5 years ago

Sounds great! Actually that issue led me here. I will keep an eye on that process.

I think in R land, since there is no actual Scalar, it is natural for PsychroLib R library to take vector inputs and I think this will cause minimum confusing for users.

hongyuanjia commented 4 years ago

@banfelder Are you still working on this?

Based on your work, I forked the psychrolib repo at hongyuanjia/psychrolib and plan to send a PR.

However, I made some changes. Your original version can be found in this commit (cf5c07b).

Change package name to psychro. Since in CRAN, 'lib' is not commonly used in package names.
Follow the coding-convetions on function names.
Make sure all functions work for vector inputs.

hongyuanjia commented 4 years ago

The PR has been sent. Please see https://github.com/psychrometrics/psychrolib/pull/49