Skip to content

Revised: Basic FlaSH Implementation #1751

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

Merged
merged 18 commits into from
Feb 6, 2023
Merged

Revised: Basic FlaSH Implementation #1751

merged 18 commits into from
Feb 6, 2023

Conversation

Ananya-Joshi
Copy link
Contributor

@Ananya-Joshi Ananya-Joshi commented Jan 6, 2023

There are some existing concerns:

  1. With linting: I'm not able to remote the lambda unnecessary comments - can you suggest a way to remove them?
  2. Some of the code is similar to the large spikes code in the validator (but has been repurposed) - what should we do?
  3. I only provided a simple test case as changes to FlaSH will probably be needed as we deploy.

The provided code is only on the evaluation side - not on generating the test statistic distribution. For more details, please see the FlaSH powerpoint on Slack (or ask me).

Of Note:
Downloading files takes a long time - I'm thinking about a way to deal with caching the relevant data frames

Any help with the above would be appreciated! Please let me know when you have a chance to discuss.

Thank you!

@Ananya-Joshi Ananya-Joshi requested a review from nmdefries January 6, 2023 15:52
@nmdefries
Copy link
Contributor

Some of the code is similar to the large spikes code in the validator (but has been repurposed) - what should we do?

I think it's fine to have duplicate or semi-duplicate code for now. It could make sense to share functions, but that would probably require a refactor of the validator, given its class structure.

I only provided a simple test case as changes to FlaSH will probably be needed as we deploy.

👍

Downloading files takes a long time - I'm thinking about a way to deal with caching

We use a caching mechanism in the CPR indicator that you can reference. It's pretty simple, We save the file to disk if it doesn't already exist, using a unique filename. Each time the indicator runs, we construct a list of new files to download.

@nmdefries
Copy link
Contributor

nmdefries commented Jan 18, 2023

Fixed the W0108: Lambda may not be necessary error in bab4e43, but linting is now showing

. env/bin/activate; pylint delphi_utils

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 9.99/10, +0.01)

. env/bin/activate; pydocstyle delphi_utils
delphi_utils/flash_eval/eval_day.py:1 at module level:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:17 in public function `fix_iqr`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:17 in public function `fix_iqr`:
        D401: First line should be in imperative mood (perhaps 'Change', not 'Changes')
delphi_utils/flash_eval/eval_day.py:30 in public function `outlier`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D209: Multi-line docstring closing quotes should be on a separate line
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D401: First line should be in imperative mood; try rephrasing (found 'An')
delphi_utils/flash_eval/eval_day.py:143 in public function `bin_dist`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:143 in public function `bin_dist`:
        D401: First line should be in imperative mood; try rephrasing (found 'A')
delphi_utils/flash_eval/eval_day.py:145 in private nested function `ts_dist`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:145 in private nested function `ts_dist`:
        D401: First line should be in imperative mood (perhaps 'Initialize', not 'Initial')
delphi_utils/flash_eval/eval_day.py:155 in public function `eval_day`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:208 in public function `return_vals`:
        D400: First line should end with a period (not 't')
delphi_utils/flash_eval/eval_day.py:208 in public function `return_vals`:
        D401: First line should be in imperative mood (perhaps 'Return', not 'Returns')
delphi_utils/flash_eval/eval_day.py:222 in public function `process_anomalies`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D202: No blank lines allowed after function docstring (found 2)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D400: First line should end with a period (not 'g')
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D202: No blank lines allowed after function docstring (found 1)
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D210: No whitespaces allowed surrounding docstring text
make: *** [Makefile:18: lint] Error 1

Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial look

Questions for in-person meeting:

  • I need a brief refresher on the general FlaSH approach.
  • Do you have an example of the alerts visualization tool? I saw a link in the code but it said the app was "asleep".
  • What broad changes have been made in this compared to the previous version of FlaSH?
  • I'm finding it hard to understand what the functions in eval_day.py are doing broadly. We can improve docstrings and maybe function names together in the meeting.


## Running FlaSH-eval

First, run the indicator so that there are files for FlaSH to check.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the production version of this downloads files from somewhere, but the local/testing version runs off of pre-existing files, sounds like this tool will need two different modes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the W0108: Lambda may not be necessary error in bab4e43, but linting is now showing

. env/bin/activate; pylint delphi_utils

-------------------------------------------------------------------
Your code has been rated at 10.00/10 (previous run: 9.99/10, +0.01)

. env/bin/activate; pydocstyle delphi_utils
delphi_utils/flash_eval/eval_day.py:1 at module level:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:17 in public function `fix_iqr`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:17 in public function `fix_iqr`:
        D401: First line should be in imperative mood (perhaps 'Change', not 'Changes')
delphi_utils/flash_eval/eval_day.py:30 in public function `outlier`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D209: Multi-line docstring closing quotes should be on a separate line
delphi_utils/flash_eval/eval_day.py:91 in public function `spike_outliers`:
        D401: First line should be in imperative mood; try rephrasing (found 'An')
delphi_utils/flash_eval/eval_day.py:143 in public function `bin_dist`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:143 in public function `bin_dist`:
        D401: First line should be in imperative mood; try rephrasing (found 'A')
delphi_utils/flash_eval/eval_day.py:145 in private nested function `ts_dist`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:145 in private nested function `ts_dist`:
        D401: First line should be in imperative mood (perhaps 'Initialize', not 'Initial')
delphi_utils/flash_eval/eval_day.py:155 in public function `eval_day`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:208 in public function `return_vals`:
        D400: First line should end with a period (not 't')
delphi_utils/flash_eval/eval_day.py:208 in public function `return_vals`:
        D401: First line should be in imperative mood (perhaps 'Return', not 'Returns')
delphi_utils/flash_eval/eval_day.py:222 in public function `process_anomalies`:
        D300: Use """triple double quotes""" (found "-quotes)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D202: No blank lines allowed after function docstring (found 2)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:244 in public function `flash_eval_lag`:
        D400: First line should end with a period (not 'g')
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D202: No blank lines allowed after function docstring (found 1)
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D205: 1 blank line required between summary line and description (found 0)
delphi_utils/flash_eval/eval_day.py:336 in public function `flash_eval`:
        D210: No whitespaces allowed surrounding docstring text
make: *** [Makefile:18: lint] Error 1

Hi Nat,

Thank you for this - I'll fix the linting bugs & we can talk about some of these high & low level comments in our meeting. FlaSH is still just a prototype, the major difference between this version and the prior version being removing some unnecessary features for the MVP. Chat soon!

Comment on lines 37 to 46
df_fix_unstack = df.ffill()
diff_df_small = df_fix_unstack.copy().diff(1).bfill()
upper = 0.75
lower = 0.25
df['day'] = [x.weekday() for x in list(df.index)]
diff_df2 = diff_df_small
diff_df2['day'] = df['day']
diff_df2_stack = diff_df2.drop(columns=['day']).stack().reset_index()
diff_df2_stack.columns = ['date', 'state', 'val']
diff_df2_stack['weekday'] = diff_df2_stack.date.dt.weekday
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of objects floating around here, some of which don't appear to be reused downstream, e.g. diff_df_small. This logic could be simplified. It's also hard for me to tell what the transformations are doing. More comments/descriptive variable names, please.

Also, when performing the first ffills and bfills, do you expect the incoming df to have missing values? Some of our indicators may produce output that don't have missing values and instead drop that index entirely. fill procedures often follow a reindexing step.

@nmdefries
Copy link
Contributor

nmdefries commented Jan 19, 2023

Notes from discussion:

  • If going to run in production, may want to exit if takes too long. Could do in FlaSH, in runner, or outside of python (in bash, cron, etc). Preference for in python code for portability/maintainability
    • Consider this secondary. For the moment, running on only 1 signal won't take too much time. And in the future we'd rather speed up the code than have to exit early.
  • Need to put FlaSH call into runner.
  • Do another run in staging and track runtime and memory usage (can query with bash script or can just keep an eye on htop). Talk to Brian about how similar staging and prod machines are, and if resource usage/runtime is comparable.
  • Possible to do trial run in production (during low-load time) to check resource usage/runtime? Ask Brian if this is possible and necessary (based on how comparable staging and production machines are).
  • More descriptive function/variable naming, and documentation. Looks like some functions could be stand-alone helpers rather than defined inside other functions.
  • Maybe split functions in eval_day.py out by topic into more .py files. One proposal: have each outlier method + helper functions in its own .py file.

Broad plan is to get this running in production as soon as possible, even if it's not totally done and not running on all signals. This will give us a better idea of where this is misbehaving (speed? some checks are oversensitive? etc).

@Ananya-Joshi
Copy link
Contributor Author

Ananya-Joshi commented Jan 22, 2023

Hi Nat, thanks for meeting.

Updates:
I/O structure:

  • runner.py updates: there is a timeout code (10 minutes) for the flash process & existing tests changed accordingly . New test not added until timeout code removed.
  • Output function in eval.py

Considerations:

  1. one of the files: last_7 contains values from the last 7 days (no more downloading). Should we keep the file locally or on AWS & access it each run? Same with the results folder, which does not save to log.
  2. Test Case : no assertion statements due to the changing nature of the last_7 file (every time the test is run, that file gets updated & creates a different result). Right now the test is that the code doesn't break. Perhaps we keep this for the first iteration, but any ideas on the last_7 file would be appreciated.
  3. For the stub implementation, only lag 1 data is considered (run.py)
  4. For out-of-range data, should the alert be in the specific indicator or in FlaSH (the support parameter). This version is only for counts data - for ratio/other types of data, some methods change, but that can be added later.

Please let me know what you think, especially about the I/O!

@nmdefries
Copy link
Contributor

one of the files: last_7 contains values from the last 7 days (no more downloading). Should we keep the file locally or on AWS & access it each run? Same with the results folder, which does not save to log.

What is being saved to AWS right now? What are the "results"?

I'm used to thinking of AWS storing only final results, and all intermediate files only being saved locally. But if the intermediate files take a long time to recreate, having AWS as a backup is convenient.

no assertion statements due to the changing nature of the last_7 file (every time the test is run, that file gets updated & creates a different result)

Tests should be deterministic. It sounds like you're saying that eval_day overwrites last_7.csv with updated data every time it is run. If that's the case, you should save the original contents of last_7 and rewrite the file at the end of the test.

last_7 = pd.read_csv("last_7.csv")
eval_day(last_7, ...)
assert(whatever")
pd.write_csv(last_7, "last_7.csv")

For out-of-range data, should the alert be in the specific indicator or in FlaSH

What does the location of the alert determine? Where it shows up in Slack?

@Ananya-Joshi
Copy link
Contributor Author

Ananya-Joshi commented Jan 23, 2023

  1. Currently, nothing is being saved in AWS. The results are the full ranking of all the streams vs. only the top 30 get written to the log that goes to Slack. But perhaps last_7 is an intermediate file.
    Action: I'll add the AWS code if the AWS credentials are passed, then save the results file in our testing bucket. [implemented]

  2. Action: Great! I'll make that testing change. [implemented]

  3. If out-of-range values are handled by each indicator separately or in the validator, then I don't want to duplicate the process. Also, I don't include out of range values in the top alerts because they are, by definition incorrect. I'm not sure what to do here.

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Ananya-Joshi, made a few performance suggestions and a few utils suggestions. Hope this feedback helps!

header=None).astype(str).set_axis(['FIPS', 'STATE'], axis=1)
fips_lookup['FIPS'] = fips_lookup['FIPS'].str.zfill(2)
fips_to_STATE = fips_lookup.set_index("FIPS").to_dict()['STATE']
STATE_to_fips = fips_lookup.set_index("STATE").to_dict()['FIPS']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This data should be available in our utils module

from delphi_utils.geomap import GeoMapper

gm = GeoMapper()
state_to_fips = gm.get_crosswalk("state_code", "state_id")
fips_to_state = gm.get_crosswalk("state_id", "state_code")
fips_pop_table = gm.get_crosswalk("state_code", "pop")
``

Copy link
Contributor Author

@Ananya-Joshi Ananya-Joshi Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can map from state code to state id! I can add the extra handling code in the future though (might need to be reused for example from geos.py in delphi_jhu.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah good point, the way to get that table is unintuitive in that util. This should work:

from delphi_utils.geomap import GeoMapper

gmpr = GeoMapper()
fips_to_state = gmpr.get_crosswalk("state", "state")[["state_code", "state_id"]].set_index("state_code").to_dict()["state_id"]
state_to_fips = gmpr.get_crosswalk("state", "state")[["state_id", "state_code"]].set_index("state_id").to_dict()["state_code"]

@@ -0,0 +1,58 @@
1,al
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these files if geo data is available to every indicator via delphi_utils.geomap?

Copy link
Contributor Author

@Ananya-Joshi Ananya-Joshi Jan 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I wasn't able to get state_id to state_code to work, does it work for you ?
"- [x] zip -> fips : population weighted
- [x] zip -> hrr : unweighted
- [x] zip -> msa : unweighted
- [x] zip -> state
- [x] zip -> hhs
- [x] zip -> population
- [x] state code -> hhs
- [x] fips -> state : unweighted
- [x] fips -> msa : unweighted
- [x] fips -> megacounty
- [x] fips -> hrr
- [x] fips -> hhs
- [x] nation"

@Ananya-Joshi
Copy link
Contributor Author

Hey @Ananya-Joshi, made a few performance suggestions and a few utils suggestions. Hope this feedback helps!

Thank you so much!

Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly questions and non-blocking suggestions. I need to come back and look at the rest of eval_day.py, but wanted you to have some comments to work off of for now.

Comment on lines 21 to 24
for signal in signals:
export_files = read_filenames(params["common"]["export_dir"])
days = {}
for (x, _) in export_files:
#Concat the data from recent files at nation, state, and county resolution per day.
if signal in x and pd.Series([y in x for y in ['state', 'county', 'nation']]).any():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking, future improvement): Since read_filenames output includes the regex match, with named substrings, we already know which signal and geo_type each file belongs to. So we can do a bit less work/fewer loop iterations here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with day below.

Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provisional approval. Have discussed with Brian and Ananya that since this is part of delphi-utils we need to release before it can be used in indicator runs in staging.

However, we expect the internals of this package to change (e.g. tuning output criteria or outlier labeling approach). The package will also need many more tests for us to be confident of its behavior.

question: Do we want this to run for all indicators? IIRC we talked about running it for only JHU (?) in which case the runner.py bit may need a step where we check the indicator being run.

@Ananya-Joshi
Copy link
Contributor Author

Ananya-Joshi commented Jan 27, 2023

Yes, it's only going to be used for JHU confirmed_cases_num at the moment. It should be good if the flash parameters are only in the jhu params.json . Would that work? I'll add the nits here ready for Monday morning! Thank you Nat!

@Ananya-Joshi Ananya-Joshi force-pushed the flash_eval branch 2 times, most recently from 33063b5 to 447d07a Compare January 29, 2023 19:30
@Ananya-Joshi
Copy link
Contributor Author

In the next iteration:

  1. use fips walkthrough
  2. use the regex to filter for signal/day

@nmdefries
Copy link
Contributor

it's only going to be used for JHU confirmed_cases_num at the moment. It should be good if the flash parameters are only in the jhu params.json . Would that work?

If flash params are entirely missing from params.json, the call to flash will fail in run.py at

def run_module(params):
    """Run the FlaSH module.

    The parameters dictionary must include the signals and signals.
    We are only considering lag-1 data.
    """
    signals = params["flash"]["signals"]

That first line will cause a key error.

I see a couple ways to address that. If we make sure that all of the indicators' params files contain a flash section with an empty signals list, then the loop won't run. Ditto if we use a dict.get() and provide an empty list as a default -- this seems like the more robust approach.

@nmdefries
Copy link
Contributor

We'd talked about caching before but I lost track of progress on it.

Downloading files takes a long time - I'm thinking about a way to deal with caching the relevant data frames

Does this need to be done before we release? How long does downloading take for just JHU confirmed_cases_num? How does it compare to our 10 min time-out?

@Ananya-Joshi
Copy link
Contributor Author

We'd talked about caching before but I lost track of progress on it.

Downloading files takes a long time - I'm thinking about a way to deal with caching the relevant data frames

Does this need to be done before we release? How long does downloading take for just JHU confirmed_cases_num? How does it compare to our 10 min time-out?

We'd talked about caching before but I lost track of progress on it.

Downloading files takes a long time - I'm thinking about a way to deal with caching the relevant data frames

Does this need to be done before we release? How long does downloading take for just JHU confirmed_cases_num? How does it compare to our 10 min time-out?

Currently all files are local and nothing needs to be downloaded! In the next iteration, we might move to having some of the parameter files on AWS, but for now they're in flash_ref

@Ananya-Joshi
Copy link
Contributor Author

@krivard I think we are good to merge sans any additional comments on this MVP!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants