Skip to content
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

Removing/data path #208

Merged
merged 97 commits into from
Apr 26, 2024
Merged

Removing/data path #208

merged 97 commits into from
Apr 26, 2024

Conversation

emprzy
Copy link
Collaborator

@emprzy emprzy commented Apr 16, 2024

Removed all relevant instances of config['data_path'] in .py and .ipynb files.

emprzy added 3 commits April 15, 2024 14:08
deleted instance of config["data_path"]
deleted instance of config["data_path"]
deleted instance of config["data_path"]
@emprzy emprzy requested review from jcblemai and alsnhll April 16, 2024 13:11
@jcblemai jcblemai linked an issue Apr 16, 2024 that may be closed by this pull request
@jcblemai
Copy link
Collaborator

  • update gempyor test config not not need datapath
  • add an error if data_path is present in the config
  • update the R code
  • update the R tests

emprzy added 9 commits April 16, 2024 13:45
Added a raise ValueError clause to catch configs that still have a data_path element
Added a raise ValueError clause to catch configs that still have a data_path element.
deleted data_path: data from config header
Deleted data_path: data from config header
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
jcblemai
jcblemai previously approved these changes Apr 17, 2024
Deleted data_path: data from config header.
emprzy added 10 commits April 17, 2024 09:37
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header.
Deleted data_path: data from config header (though, it was only commented out).
Deleted data_path: data from config header.
Deleted data_path: data from config header.
emprzy and others added 10 commits April 18, 2024 09:00
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Prepended the mobility object with the correct path (mobility.csv -> data/mobility.csv).
Added a raise ValueError clause to catch if the config being used still has a data_path section. Commented out line 579 because that portion contains config["data_path"], but the spatial_base_path variable it is read into is not referenced anywhere else in this file.
jcblemai
jcblemai previously approved these changes Apr 19, 2024
Copy link
Collaborator

@jcblemai jcblemai left a comment

Choose a reason for hiding this comment

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

All good, I need to think if we need to update the R code as well before merging

First attempt at removing instances of `config$data_path` from R files.
datasetup/build_US_setup.R Show resolved Hide resolved
datasetup/build_covid_data.R Show resolved Hide resolved
datasetup/build_covid_data.R Show resolved Hide resolved
datasetup/build_flu_data.R Show resolved Hide resolved
flepimop/main_scripts/inference_slot.R Show resolved Hide resolved
flepimop/main_scripts/inference_slot.R Show resolved Hide resolved
Copy link
Contributor

@saraloo saraloo left a comment

Choose a reason for hiding this comment

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

Looks good to me. Re: @jcblemai 's comment on fixing the output directory: if we want to fix the directories just note that any covid or flu data should be in the data folder as we have defined them - and geodata and mobility in the model_input folders.

I'm not sure we need to define the outdir in any case as it should save to the full relative path anyway.

@jcblemai
Copy link
Collaborator

I'm not sure we need to define the outdir in any case as it should save to the full relative path anyway.

I think we do so we can create the directory if it does not exist. outdire should just be fixed.

@emprzy If sara's agree some changes are:

  • outdir <- 'data' in build_flu_data and build_covid data, then keeping it throughout the code (undoing the change done in this files)
  • outdir <- 'model_input' for build US and non US files, and keeping it throughout the code

The rest is all good to me :)

@saraloo
Copy link
Contributor

saraloo commented Apr 25, 2024

Ah yes I see the create directory part.

Propose the following change:

  • instead of setting the directory (which in our case is fine because these are just files we use for covid and we have these folders already set) you could replace the lines that define the directory and create it with the following, replacing whatever the file path in the config is. eg
    if(!dir.exists(dirname(config$subpop_setup$geodata))){dir.create(dirname(config$subpop_setup$geodata))}
    and the equivalent for the mobility or data files

Either method work though, and everything else is also good to me.

@jcblemai
Copy link
Collaborator

agree with @saraloo sara. this is way to go @emprzy

emprzy added 2 commits April 25, 2024 14:22
Added a clause to create the appropriate directory (geodata, mobility, or gt_data) in datasetup R files.




# MOBILITY DATA (COMMUTER DATA) ------------------------------------------------------------


if(state_level & !file.exists(paste0(config$data_path, "/", config$subpop_setup$mobility))){
if(state_level & !file.exists(paste0("/", config$subpop_setup$mobility))){
Copy link
Contributor

Choose a reason for hiding this comment

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

This also should be changed in line 172
You'll have to remove the paste0('/' as well as this will look for the file in a different place.
ie
file.exists(config$subpop_setup$mobility)

Removed references to config$data_path and "/" in paste0 functions.
Copy link
Collaborator

@jcblemai jcblemai left a comment

Choose a reason for hiding this comment

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

This looks good to me now

@saraloo saraloo merged commit 40207b7 into HopkinsIDD:main Apr 26, 2024
1 check passed
jcblemai pushed a commit that referenced this pull request Oct 3, 2024
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.

Remove or Restructucture "data_path" option and use in config
3 participants