-
Notifications
You must be signed in to change notification settings - Fork 32
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
Bubble plot visualization #642
Bubble plot visualization #642
Conversation
@hayleefay Thanks for setting up this PR. First, I think you need to add a blank Second, I tried to run the app locally and got the following error: Did I do something wrong here? I used your code as is except for adding the |
@hdoupe Ah, that is probably a result of the version of bokeh being used. I forgot to include that in the PR. This visualization was built on 0.12.4. I updated |
@hayleefay Great, thanks! I see what you mean. So, the javascript calls |
@hdoupe Actually, when I increased the time from 7 to 15 it loads. Is your page not loading at 15? |
@hayleefay, I have a question about the following line in this pull request:
Is this being added because the bubble plot cannot be created using bokeh 0.12.3? |
@hayleefay, I realized my problem. I'm not sure what's happening here. Let me look into this some more. |
@hdoupe In order to get the other keys, you'll need to run the newest B-Tax version from Github. Jason made some changes last week to update what B-Tax is passing. |
@martinholmer The visualization actually can be loaded on 0.12.3. It just can't be loaded on 0.12.6 and when I run locally that is the version it is defaulting to if I don't pin a version. Will that not be an issue in production? |
@hayleefay Ah, right. Now it's working. I set up the local version of btax but forgot to remove the btax in my |
@hayleefay said:
I'm confused. I thought TaxBrain used packages. The taxcalc package, the btax package, and the ogusa package. The newest btax package available at anaconda.org is 0.1.8, which is more than two months old. Where are the new btax packages? |
@martinholmer said
We are using the most up-to-date code in the B-Tax repo. But there will need to be a new B-Tax package on anaconda.org in order to put this on the test and production servers. |
@hayleefay said:
This situation indicates a serious problem. I don't know if there has been a regression moving from bokeh 0.12.3 to 0.12.6 or if there is something about your code that is inconsistent with bokeh 0.12.6. Pinning to 0.12.4 is just sweeping the dust under the rug. You need to figure out what enhancement (or bug?) in 0.12.6 prevents your bubble plot from working. This is very important. |
@martinholmer, okay, I will work on updating the code. I see that 0.12.7 was just released. Should I update the code (if needed) for 0.12.7 or stay on 0.12.6? |
@hayleefay I don't have much javascript experience, but I think a way to do this would be to leave the 7 second interval as is. Then, when Something along these lines:
Then, change the javascript to: There are some details that you would have to fill in, but I think something like this would work. |
@martinholmer, I have updated the code to work on the latest version, 0.12.7. It looks like we have to pin the version in order to load the BokehJS for the widgets: https://bokeh.pydata.org/en/0.12.7/docs/user_guide/embed.html Or do you know a way around this? |
@hayleefay said in pull request #634:
Admittedly, I know very little about the bokeh package, but I'm very confused. Originally, I thought you said your pull request worked with bokeh 0.12.3 or 0.12.4, but now you're saying it needs the very latest version (0.12.7) in order to work. Which is it? Can you explain which enhancement is required by your pull request on this list of 0.12.7 enhancements? In other words, what is missing from earlier bokeh releases that your pull request needs? Which commit contains the changes necessary for the plot "to work on the latest [bokeh] version, 0.12.7"? And if 0.12.7 is an absolute requirement for this pull request, why does the
|
@martinholmer Originally, my PR did work with Bokeh 0.12.4/0.12.3. Because of your feedback, I updated the code to 0.12.7. I have yet to push it because I was unsure of how to update the CDN for the widgets in Only small changes in handling null values were required to change from 0.12.4 to 0.12.7. However, the code does not work in 0.12.6 due to errors in the handling of Categorical data, which were fixed in the release of 0.12.7. Can you tell me what you're hoping for as far as versioning? |
@martinholmer @hayleefay the place to specify this is in https://github.com/OpenSourcePolicyCenter/webapp-public/blob/master/deploy/fab/dropq_environment.yml This file is used for the dev environment and the deployment environment. |
requirements.txt
Outdated
@@ -27,3 +27,4 @@ boto | |||
django-storages | |||
django-htmlmin | |||
pyparsing | |||
bokeh==0.12.4 |
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.
Need to remove this line and add specify the version in dropq_environment.yml
.
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. Thanks for the helpful info. But then what role does requirements.txt
play in TaxBrain?
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.
This file is for installing packages from pip. It's mostly Django specific packages. Ideally we could be using the one environment file, and get rid of all these requirements files. environment.yml
files can also install pip packages. This would be a good bit of code cleanup, but it would require some refactoring of the deployment scripts.
In the case of Bokeh, since its Continuum run, its best to install it from conda.
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.
@brittainhard, Thanks for the explanation.
@brittainhard Thanks for the tip, I have made that change! |
@hdoupe Thanks so much. I implemented the new |
@brittainhard @martinholmer @jdebacker Is this ready to be merged into @brittainhard's PR #634? We would love to have this visualization up on the site as soon as possible. But I know that involves merging #634 and releasing a new version of B-Tax. Any time frame on those things? |
webapp/apps/btax/views.py
Outdated
exp_num_minutes = dt.total_seconds() / 60. | ||
exp_num_minutes = round(exp_num_minutes, 2) | ||
exp_num_minutes = exp_num_minutes if exp_num_minutes > 0 else 0 | ||
JsonResponse({'eta': exp_num_minutes, 'wait_interval': 15000}, status=202) |
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.
Great work here @hayleefay. I just have one comment. I don't think we need to re-calculate the wait time here since we know that it will be about 15 seconds. If you want you could remove 327-333 and replace exp_num_minutes with .25.
Sorry I meant to leave this comment a couple days ago.
@hdoupe, good idea! I pushed that change. |
@hayleefay Looks good, thanks! |
@hayleefay I just tagged a new release of B-Tax (0.1.9) that has the changes you need. I'd have to look back to read over my notes about how to package up a new version. It may be the evening of 9/6 before I can get to this. |
@jdebacker said:
The package creation process in the |
@hayleefay I was swamped with class prep then had some issues building a package (first time I've done it), but I was able to successfully complete the process (I think) and you can find a new version of B-Tax on Anaconda.org now. |
@jdebacker, no worries! Thanks for getting that done. @brittainhard, when do you plan on deploying the new version of webapp-public? |
@@ -4,7 +4,7 @@ dependencies: | |||
- pandas>=0.20.1 | |||
- flask | |||
- greenlet | |||
- bokeh | |||
- bokeh>=0.12.7 |
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.
This needs to be set in deploy/fab/drop_environment.yml
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.
This is deploy/fab/drop_environment.yml
🙂
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.
Oops!
@hayleefay the plan is to get PRs merged and do testing today and monday, then deploy to the dev node end of day monday. Is this ready to be reviewed? |
@brittainhard, sounds great! And yes it is! Let me know if I need to make any changes. |
@hayleefay this looks great. What version BTAX do we need for this? |
@brittainhard 0.1.9 |
@hayleefay @jdebacker merging this. |
This PR includes a bokeh bubble plot that will appear on the results page of the ccc.