-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
[WIP] PolicyBrain API #354
Conversation
@jdebacker This looks good so far. I quickly sketched out how I think an OG-USA parameters class would look on this branch. It will need to be filled in and integrated with OG-USA, but for the purposes of reasoning about the API, I think it should do. What do you think? Do you think that type of approach could work? |
@hdoupe Thanks for your work in the other branch. I have a number of questions. The first two are:
|
I think moving the hard-coded parameters into the I think that instead of doing all of the parameter validation and processing in places like this in A call sounds good--just sent you a message to set that up. |
"out_of_range_maxmsg": "", | ||
"out_of_range_action": "stop", | ||
"compatible_data": {"puf": true, "cps": true} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jdebacker how is MINIMIZER_OPTIONS
used? I did a search in the code base and can see where it's set but not where it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hdoupe You are right - it is not used. I'll drop this from the JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, sounds good.
Deleted unused Added @hdoupe's work on |
@jdebacker this looks good so far. I put some thought into some of the questions you posed:
i, iv, vi) We may have to do the initial range validation and then do further validation on these more complex requirements. v) Is this something that we could specify with the parameters in the JSON files? If so, could you just create a JSON file like the Tax-Calculator presets that specifies the test parameters? vii) Would it make sense to include those in the viii) I feel like the baseline boolean parameter could be included in the iii) I was able to get a working solution for this which allows you to set the range for a parameter like:
where f or g is either: All of this is on my ogusa-paramsbase branch. The function that extends the range capabilities is here and it's called from here. I need to spend some time cleaning up the code, documenting, and adding a test or two. Then, I could open up a PR on this branch that you are working on. Opening a PR on a PR is kind of a clumsy way to integrate changes, but I don't know of a better way to do it. If you want to try another process, then we can talk about that. |
See jdebacker#9 for further implementation of the |
@jdebacker Anything I can do to help move this along? Or are there things listed in #352 that I could do independent of the work happening here? |
@asmockler As you can see, my work on this branch has been intermittent and I just set out the basics of the parameter defaults and so on as outline by @hdoupe. I feel like I need to think more about how OG-USA needs to be restructured to work better with the PolicyBrain API and forthcoming changes to PB. At the moment, I'm having hard time thinking of places to make small independent contributions because I don't have an exact idea what the final structures should look like. Let's put any discussion over in Issue #352 or new issues. |
This PR attempts to bring OG-USA into line with the PolicyBrain API using Issue #352 as a guide.