Refactor (move towards module)

[sam-writer]

Closes #19

Makes progress towards #4

Closes #76 (or maybe more documentation is needed to fully close #76 - ping @mariekers )

Also, IMO, makes future changes easier by separating (as much as is possible with Streamlit) presentation and logic.

It would be nice to get confirmation from a Data Scientist and a more Front-endy person that these changes work for them. Also, sorry if this goes too far, I'm not trying to be a jerk. If we are going to make this pip-installable, per #4, a lot of this seems needed to me.

[sam-writer]

@luminoso I carefully read your comments on #4 just now, I hope I didn't step on any work you are doing. If you did something that overlaps, maybe we can coordinate how to merge the two?

[sam-writer]

Also, this is forked from #54 , so hopefully I fixed those merge conflicts right 😬

[mdbecker]

@sam-qordoba I noticed some of the checks failing (specifically pytest). I assume this is expected but let me know if it is not.

[luminoso]

is there any reason to include N parameter in def sir(y, beta, gamma, N): function @ models.py? Because CHIME.py invokes sim_sir() and it defaults N to N = S + R + I. when sir() is invoked you just compute always that scale = 1

IMO, Nshould be removed from sir() for three reasons:

• it doesn't belong to the original formula on the wikipedia
• in the current implementation it is always computing scale = 1
• scale can be computed outside sir() for whichever function that needs it in the future

[sam-writer]

@mdbecker that looks like a placeholder test to me - should I change the test?

[sam-writer]

is there any reason to include N parameter in def sir(y, beta, gamma, N): function @ models.py? Because CHIME.py invokes sim_sir() and it defaults N to N = S + R + I. when sir() is invoked you just compute always that scale = 1

IMO, Nshould be removed from sir() for three reasons:

• it doesn't belong to the original formula on the wikipedia
• in the current implementation it is always computing scale = 1
• scale can be computed outside sir() for whichever function that needs it in the future

@luminoso I agree with this, but I think it should be reported as a separate ticket. This PR is already much bigger than I like, and that is a semantically distinct action item.

EDIT: I made a ticket, #84

[sam-writer]

This PR is back to being reading to merge. I fixed the merge conflicts, and got the new code merged in 🤞

Summary of changes since last night

So, to minimize changes, I renamed it back to app.py - we can have a seperate PR for renaming the file.

I added the manual testing policy as a MD file, and will run through it one last time before signing off.

I added some actual tests to test_app.py, just a few, but better than nothing.

The only changes, on top of the refactor, are

• I tried to follow @themightychris' instructions for the Procfile, since they made sense to me (unable to connect heroku app to frontend router via new toml files. #64, comments)
• Changed calls from st.dataframe to st.table - this makes the tables a little more spartan, but at least on all the devices I could test, MUCH more readable.

Overall summary of this PR

• Create directory penn_chime for modules. Originally called this chime, but the root is also chime, and the root has an __init__.py in it, and that is dangerous, you don't want from chime.chime import ...
• Extract DataFrame builder functions into penn_chime.utils
• Extract presentation code (Streamlit and Altair) into penn_chime.presentation. This involved encapsulating everything in closer-to-pure functions, so I had to make them take as arguments all the variables they expect (rather than having everything be global).
• Extract models into penn_chime.models - this was the best part, thank god math functions are usually pure
• Added a few unit-ish tests, mostly to provide a template for others
• Changed the invocation of pytest in the GH Action from pytest to python -m pytest

There is still some logic mixed with presentation in app.py, and I am open for ideas on how to improve it further, but IMO it is now much easier to keep work separate.

[sam-writer]

Here is what I mean about displaying the tabular data better:

[sam-writer]

Actually, this fixes #75 as well, except for a few E501s, which I think are unavoidable as we are writing markdown directly in Python. I set up my local editor to ignore this one error type, if it comes from the presentation.py file. Proof from latest run of Actions:

We can tell the linter not to care about this one error, but in general it is an okay rule. @garysieling does that work for you?

[mdbecker]

Just completed a second pass on the manual tests. Found one minor merge error which I'll push a fix for shortly. Merge incoming after a very quick code review.

[fbenamy]

This undid the graph granularity issue. The good news is that I can very easily fix it in the code and will merge a code review for it shortly.

[mdbecker]

Hi @fbenamy. It must be the lack of sleep but I'm not following how the granularity issue pertains to this PR. Let's discuss on slack if you have a chance. I'm going to merge this soon if I could get one more review That would be great!

[mdbecker]

I'm worried about the Procfile changes but we need this immediately so I approve.

[mdbecker]

After talking with @fbenamy it sounds like we might have missed something from master in this PR. Working on that now.

[fbenamy]

The refactoring looks great.

[mdbecker]

Looks great! Unbelievable work everyone!