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

Get/Set Storage Nodes Parameters #192

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

joaolponciano
Copy link

@joaolponciano joaolponciano commented Aug 13, 2018

Closes #189

Hello Bryant,

Here are the modifications we was talking about. I'm not so sure about the units.
As I told you before this is the first time I am working in GitHub, so I might have made some mistakes, I'm sorry about them. Thanks for your help and attention.

Thanks,
João.

Copy link
Member

@bemcdonnell bemcdonnell left a comment

Choose a reason for hiding this comment

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

@joaolponciano, please migrate all this code to the toolkitAPI.c module. It appears that you have a syntax error somewhere as well. In response to your comment on Units, look at how other getter/setters work. When you set a value, the units are converted on the way in. when you get a value, the units are converted on the way out. Make sure you multiple.divide correctly. I also believe that there are three parameters that make up a functional storage curve (you seem to only have aCoeff). Can you check that?

@bemcdonnell
Copy link
Member

@joaolponciano, also please sign the contributors license agreement HERE

@dickinsonre
Copy link

There are three parameters in a storage function

Area = A + B^C though you can get by just using A.

@joaolponciano
Copy link
Author

@bemcdonnell, thanks again for the help. I checked the points you told me to, and corrected them, thanks. I'm just checking the syntax yet. To migrate the code for toolkitAPI.c should I close this pull request and create another one or it's possible to put the code here?

@bemcdonnell bemcdonnell changed the base branch from toolkitapi to develop August 15, 2018 18:39
@bemcdonnell bemcdonnell added this to the v5.2.0.dev4 milestone Aug 15, 2018
@bemcdonnell
Copy link
Member

@joaolponciano, just move the code (please put your functions around similar functions for nodes), delete the toolkitAPImodified.c file, and make another commit with these changes. There are going to be lots of additional commits that will ride inside your PR.

Once we agree on the function "themes", you'll need to add unit tests as well so we can always be sure this code is working. You'll also need to add your function declarations to /include/toolkitapi.h

Once we are all satisfied with the implementation and tests, we will approve the PR and your code will come into SWMM. Also, at this point, go ahead and append your name to the AUTHORS file so we can more easily associate your changes with you.

@joaolponciano
Copy link
Author

@bemcdonnell, I think the last commit I uploaded meets the first requests you did. Should I add the function declarations to /include/toolkitapi.h already?

Thanks for all your help.
João

@bemcdonnell
Copy link
Member

@joaolponciano You code is very far out of date based on where the OWA develop branch is. Could you rebase your work please? Also it appears to be failing to build on Appveyor.

@joaolponciano
Copy link
Author

@bemcdonnell, Should I rebase them to develop? And I will check what is failing in Appveyor.

Thanks.

@bemcdonnell bemcdonnell modified the milestones: v5.2.0.dev5, v5.3.0.dev0 Jul 30, 2019
@bemcdonnell
Copy link
Member

@joaolponciano, Sorry that things got really busy 5 years ago! @karosc, I am going to bring this PR back to life. It is a really great contribution!

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

Successfully merging this pull request may close these issues.

add storage node get/set (Setting functional curve coefficient)
3 participants