Update README.md to reflect 2024 changes#86
Conversation
kyrasturgill
changed the title
| name = "Total Tax Amount", | ||
| labels = scales::dollar, | ||
| expand = c(0, 0), | ||
| expand = expansion(mult = c(0, .05)), |
There was a problem hiding this comment.
With the dramatic 2024 increase I thought the axis spacing was making it look as though it was cut off, so I added some more buffer here.
There was a problem hiding this comment.
[Question, non-blocking] It's a bit hard to tell since the Y-axes are not aligned, but it looks as if the increment value is now larger between 2016 and 2023 than it was in the previous version of the chart, even setting aside the big jump in 2024 (which is also pretty shocking, so much so that we should fact check it). Do you see that too? If so, any idea why might be the case?
There was a problem hiding this comment.
I see that too when looking at the diff - this .png was generated when I knitted the file but when I am running that code chunk alone it appears correct and aligned with the old viz. Weird, not sure why that is happening!
Here is image I get when I just ran it:
And as for the dramatic spike, this PIN's AV spiked in 2024, going from 7000 to 12500. Perhaps we should select a PIN with more stable AV?
There was a problem hiding this comment.
this .png was generated when I knitted the file but when I am running that code chunk alone it appears correct and aligned with the old viz. Weird, not sure why that is happening!
That's so strange! Let me know if you want a second eye debugging it. I get the correct and more aligned version when I run it and knit it locally too, although I have to tweak the two DBI::dbConnect() paths to point to the correct database -- I wonder if it's possible that you have two different versions of the database, one of which gets loaded instead of the other when knitting vs. running locally? Very low confidence in that diagnosis, it's just an idea as to the kind of thing that might be going wrong. In any case, I think we should make sure we have the correct / aligned version of the chart before we merge.
And as for the dramatic spike, this PIN's AV spiked in 2024, going from 7000 to 12500. Perhaps we should select a PIN with more stable AV?
Got it, that makes sense! I don't mind keeping it as is, it's kind of interesting to see such a dramatic spike in 2024.
There was a problem hiding this comment.
I think I have addressed the problem. When knitting the doc it was loading the existing version of the ptaxsim package rather than the dev version which meant TIF amounts were not being calculated correctly. I knit the doc with it loading the dev version and pushed the updated viz .png and .md files but I'm not 100% sure that was the right way to do it
jeancochrane
left a comment
jeancochrane
left a comment
There was a problem hiding this comment.
Nice work here! I'm glad this is so simple. There's only one issue with the small diagram that I think is blocking; otherwise, consider everything else below to be suggestions.
| name = "Total Tax Amount", | ||
| labels = scales::dollar, | ||
| expand = c(0, 0), | ||
| expand = expansion(mult = c(0, .05)), |
There was a problem hiding this comment.
[Question, non-blocking] It's a bit hard to tell since the Y-axes are not aligned, but it looks as if the increment value is now larger between 2016 and 2023 than it was in the previous version of the chart, even setting aside the big jump in 2024 (which is also pretty shocking, so much so that we should fact check it). Do you see that too? If so, any idea why might be the case?
| double transit_tif_to_cps | ||
| double transit_tif_to_tif | ||
| double transit_tif_to_dist |
There was a problem hiding this comment.
[Nitpick, optional] I wonder if these transit_tif_to_* fields are even worth including in this small version of the diagram? They're definitely important, but I also think we could cut them for further simplicity.
There was a problem hiding this comment.
I was debating this as well, I am fine with removing
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
Co-authored-by: Jean Cochrane <jeancochrane@users.noreply.github.com>
|
@jeancochrane I believe I implemented all requested edits - only thing I'm unclear on is why the viz is off when the doc is rendered. |
There was a problem hiding this comment.
Looks great, barring one small change to the fund_type_num definition in create_db.sql that I can't quite explain! Let's plan to pair on Tuesday to see if we can figure out why the knitted chart looks different from the one that renders locally. Once we get that resolved, this is good to merge.
| CREATE TABLE agency_fund_info ( | ||
| agency_num varchar(9) NOT NULL, | ||
| fund_type_num varchar(3) NOT NULL, | ||
| fund_type_num varchar(6) NOT NULL, |
There was a problem hiding this comment.
[Question, non-blocking] Was this change intentional?
There was a problem hiding this comment.
I'm losing it, I must have ready this as fund_num and thought I'd made a mistake. I will revert this change.
jeancochrane
left a comment
jeancochrane
left a comment
There was a problem hiding this comment.
Thanks for getting to the bottom of the chart rendering issue!
| fund_name varchar NOT NULL, | ||
| capped_ind boolean NOT NULL, | ||
| PRIMARY KEY (agency_num, fund_type_num, fund_num) | ||
| PRIMARY KEY (agency_num, fund_num) |
There was a problem hiding this comment.
[Question, non-blocking] Just to clarify: I believe we will need to do a new database export to reflect this change, correct? It's not a big deal -- the most important advantages of primary keys are uniqueness constraints and indexes, neither of which is particularly important for this use-case -- but I just want to make sure my understanding is correct.
There was a problem hiding this comment.
Yep that's correct! Which we will ultimately need to do now anyway to incorporate the new agency crosswalks.
2 participants
Necessary README updates to reflect the 2024 update. Not a ton to be changed here as I thought there would be. I opted to keep most things the same, barring where an update was required. Most substantial change is to the database diagrams, which I hope will render correctly!