-
Notifications
You must be signed in to change notification settings - Fork 4
Update README.md to reflect 2024 changes #86
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
Changes from all commits
ed7761d
149303e
44f87de
60b3833
ad3fe1f
153ced3
e70efd0
19c9a66
ecfbf27
21f0615
4d742ca
0e16ca1
e0fab39
6e99471
b766eb5
dc599c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ CREATE TABLE agency_fund_info ( | |
| fund_num varchar(6) NOT NULL, | ||
| 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) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that's correct! Which we will ultimately need to do now anyway to incorporate the new agency crosswalks. |
||
| ) WITHOUT ROWID; | ||
|
|
||
| CREATE INDEX ix_agency_fund_info_capped_ind ON agency_fund_info(capped_ind); | ||
|
|
||
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have addressed the problem. When knitting the doc it was loading the existing version of the
ptaxsimpackage 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