search

QSD-Group / QSDsan

Quantitative Sustainable Design (QSD) of sanitation and resource recovery systems.
https://qsdsan.com
Other
30 stars 14 forks source link

Use biosteam correlation methods for better code-reuse #28

Closed yoelcortes closed 3 years ago

yoelcortes commented 3 years ago

Hi Yalin,

This pull request implements changes to the get_correlation function for better code-reuse. The API is intact, so it should be compatible with any tests. This change also speeds up the get_correlation function significantly, although it didn't take that long to begin with (on the order of milliseconds).

Note that BioSTEAM has no plans to implement more complicated sensitivity analyses as part of it's API, but might include a tutorial on this down the line.

Although BioSTEAM also has a sampling method for convenience, I made no changes to the qsdsan's generate_samples function as it doesn't accept the model object as a parameter.

Let me know what you think, Thanks!

yoelcortes commented 3 years ago

Although all pytests passed on my computer, QSDsan has a specific biosteam version requirement, but this pull request requires biosteam v2.24.10. I'll leave this pull request open until the version requirement is updated.

Thanks!

yalinli2 commented 3 years ago

Hi Yoel, thanks for the pull request! I'll take a look and get it into QSDsan, although I might not have time until the weekend, thanks!

yalinli2 commented 3 years ago

Hi Yoel, I think I somehow messed up this PR a bit. I made some updates to the stats module (in my fork) since you forked the root repo. I thought it'd be easier if I pulled in my commits before merging yours. When I was resolving the conflicts between the updated root repo and your fork, I thought it'd be good to include the authorship guideline and author list. However after I made all those changes and pushed them to the root repo, it looks like GitHub isn't recognizing this as merging your PR (probably because I added in other changes...).

So I'll close this PR, but you are now acknowledged in the stats module. Let me know if there's a way to get this PR merged properly and I'll fix it, thanks!!!

yoelcortes commented 3 years ago

@yalinli2, that's OK. FYI, you can create a new branch from someone else's fork and then merge. This keeps the commit history, marks the contribution on github, and makes it easier for third-party contributors pull your changes too. This thread should help: https://stackoverflow.com/questions/5606048/git-merge-from-someone-elses-fork

I would also recommend you and Joy work on different branches within the QSDsan repository (not the forks). This makes it easier to merge, checkout branches, and accept pull requests.

Thanks!

yalinli2 commented 3 years ago

@yoelcortes Oh the thread is very helpful, thanks for sharing that!

For branch vs. fork, why using branch will be easier to merge? I read some posts like this (https://github.community/t/branch-vs-fork/1206), but still not sure about the real differences. Thanks!

yoelcortes commented 3 years ago

Well, the easier part comes in with not having to fetch the third-party fork as a branch (note the third party cannot work on the branch you just fetched). If you are members of the same organization, best you use branches (not forks). Then you could work on multiple branches together and anyone can merge whenever it's appropriate. For example, Joy finished making some important changes and decides to merge her branch to yours (so that you can keep working on your unfinished feature with her changes).

yalinli2 commented 3 years ago

Oh I see, this sounds good and we will consider it, thanks!