Med obs model #139

galengorski · 2022-06-17T18:16:34Z

Ok this is ready for review now. I realized that the error that I was getting was because I wasn't getting the model to retrain, so I have explicitly run the train_model step in the snakemake file with the -f forcerun command. I'm sure there is a more elegant and flexible solution to this.

lekoenig

Thanks, @galengorski! I haven't run or commented on the changes to the snakemake files, but this looks good to me!

lekoenig · 2022-07-28T17:40:19Z

2a_model/src/models/0_baseline_LSTM/config.yml

-x_vars: ['seg_ccov', 'seg_rain', 'seg_slope', 'seg_tave_air', 'seginc_swrad', 'hru_slope', 'hru_aspect', 'hru_elev', 'hru_area', 'hru_percent_imperv', 'covden_sum', 'covden_win', 'soil_moist_max']
+site_set: "well_obs"
+
+x_vars: ['pr','SLOPE','tmmx','srad','CAT_BASIN_SLOPE','CAT_ELEV_MEAN','CAT_BASIN_AREA','CAT_IMPV11', 'CAT_CNPY11_BUFF100','CAT_TWI']


Ahh! This makes a lot more sense now. We should probably add a comment somewhere in 2a_model.R (maybe above p2a_med_obs_data and p2a_well_obs_data) that reminds a user that x_vars needs to be updated in this config file if any variables are added/omitted.

Good idea! I added a comment in 2a_model above p2a_well_obs_data

lekoenig · 2022-07-28T17:41:21Z

2a_model/src/models/Snakefile_base.smk

@@ -100,7 +101,7 @@ rule make_predictions:
    input:
        "{outdir}/prepped.npz",
        "{outdir}/nstates_{nstates}/nep_{epochs}/rep_{rep}/train_weights/",
-        "../../../out/well_obs_io.zarr",
+        "../../../out/"+site_set+"_io.zarr",


I like this addition to make the "site set" more flexible.

lekoenig · 2022-07-28T20:11:17Z

One more note, @galengorski - do you mind linking issue #122 to this PR so that it gets automatically closed whenever you merge? You can do that by clicking the 'development' cog in the upper right corner of this PR page and selecting the relevant issue.

jsadler2

Nice work, @galengorski.

I have a few comments and I wanted to touch base about your changes to the 2a_metrics_files target before merging.

jsadler2 · 2022-08-05T20:11:05Z

2a_model.R

         # the 1_ models use the same model and therefore
         # the same Snakefile as the 0_baseline_LSTM run
-         list(model_id = "1_metab_multitask",
+         # list(model_id = "1_metab_multitask",


Why did you comment these out? Did you try them and they didn't work?

Ah, just so I could specifically run the baseline models, I will uncomment them

jsadler2 · 2022-08-05T20:24:48Z

2a_model.R

+    weights_trained_file <- file.path("../../../out/models",p2a_model_ids$model_id,"nstates_10/nep_100/rep_0/train_weights")
+    output_feather_file <- file.path("../../../out/models",p2a_model_ids$model_id,"nstates_10/nep_100/rep_0/preds.feather")

    # First create the prepped data files if they are not already.
    # These are needed to make the predictions.
-    system(stringr::str_glue("snakemake {prepped_data_file} -s {snakefile_path} --configfile {config_path} -j"))
+    system(stringr::str_glue("snakemake {prepped_data_file} -s {snakefile_path} --configfile {config_path} -j -f"))

+    system(stringr::str_glue("snakemake {weights_trained_file} -s {snakefile_path} --configfile {config_path} -j -f"))
+
+    system(stringr::str_glue("snakemake {output_feather_file} -s {snakefile_path} --configfile {config_path} -j -f"))
+
    # Then touch all of the existing files. This makes the weights "up-to-date"
    # so snakemake doesn't train the models again
-    system(stringr::str_glue("snakemake -s {snakefile_path} --configfile {config_path} -j --touch"))
+    #system(stringr::str_glue("snakemake -s {snakefile_path} --configfile {config_path} -j --touch"))

    # then run the snakemake pipeline to produce the predictions and metric files
-    system(stringr::str_glue("snakemake -s {snakefile_path} --configfile {config_path} -j --rerun-incomplete"))
+    system(stringr::str_glue("snakemake -s {snakefile_path} --configfile {config_path} -j -f"))


If I'm understanding this correctly, one problem with this approach is that it will rerun the full Snakemake pipeline the p2a_metrics_files target needs to be run. For example, if I clone this onto my machine, it won't have the metrics file for exp 0, so targets will trigger the rebuilding of that branch of the p2a_metrics_files. Then it will

build the prepped data file, which good (line 142)

retrain the rep_0 model (line 143)

make the rep_0 preds (line 144)

build the overall metrics files (line 159) which would include making all of the predictions (unless they are already there, like rep_0 (line 144))

I think the reason you added these lines in here was to force a retraining of the model because the previous trained model weights were different than the new, NHD ones. Is that right? I think a cleaner way to do that would be to just delete the previous model weights and run this target as it was written before. That should trigger a retraining of all of the model replicates etc. Did you try that?

Oh yeah, good point! I think this makes sense, I am going through and testing it to make sure it works. One odd behavior that I am running into: When I got through and delete the weights out/models/0_baseline_LSTM/nstates_10/nep_100/rep_0/train_weights, then rerun p2a_metrics_files using the original set of snakemake commands (prepped.npz,touch, and rerun incomplete), the model produces a new train_weights file for all model ids except 0_baseline_LSTM. Even if I delete all the model output, everything within out/models/0_baseline_LSTM/nstates_10, I still don't get the model to retrain

Strange! So you don't have any outputs at all for 0_baseline_LSTM/nstates_10?

jsadler2 · 2022-08-05T20:43:05Z

2a_model/src/models/Snakefile_base.smk

@@ -29,7 +30,7 @@ rule as_run_config:

 rule prep_io_data:
    input:
-        "../../../out/well_obs_io.zarr",
+        "../../../out/"+site_set+"_io.zarr",


Maybe personal preference, but I think this is a little be cleaner. Using Python's string formatting. That way you don't have to open and close the quotes and I think it's a little less error prone since it's harder to accidentally add in an extra space.

Suggested change

"../../../out/"+site_set+"_io.zarr",

f"../../../out/{site_set}_io.zarr",

jsadler2 · 2022-08-05T20:44:00Z

2a_model/src/models/Snakefile_base.smk

@@ -100,7 +101,7 @@ rule make_predictions:
    input:
        "{outdir}/prepped.npz",
        "{outdir}/nstates_{nstates}/nep_{epochs}/rep_{rep}/train_weights/",
-        "../../../out/well_obs_io.zarr",
+        "../../../out/"+site_set+"_io.zarr",


Suggested change

"../../../out/"+site_set+"_io.zarr",

f"../../../out/{site_set}_io.zarr",

jsadler2 · 2022-08-05T20:44:32Z

2a_model/src/models/Snakefile_base.smk

@@ -174,7 +175,7 @@ def get_grp_arg(wildcards):

 rule combine_metrics:
     input:
-          "../../../out/well_obs_io.zarr",
+          "../../../out/"+site_set+"_io.zarr",


Suggested change

"../../../out/"+site_set+"_io.zarr",

f"../../../out/{site_set}_io.zarr",

jsadler2 · 2022-08-05T20:44:56Z

2a_model/src/models/visualize_models.smk

 rule make_obs_preds_plots:
    input:
        pred_file="{outdir}/nstates_{nstates}/nep_{epochs}/rep_{rep}/preds.feather",
-        obs_file="../../../out/well_obs_targets.zarr",
+        obs_file="../../../out/"+site_set+"_io.zarr",


Suggested change

obs_file="../../../out/"+site_set+"_io.zarr",

obs_file="../../../out/{site_set}_io.zarr",

…date weights

galengorski force-pushed the med_obs_model branch 2 times, most recently from 70cdc92 to 0ea015e Compare June 29, 2022 18:17

galengorski added 4 commits July 1, 2022 10:01

commit test for hpc pull

67d430f

add targets/functionality for moderately observed sites

ca51af8

add directory for 3_baseline_med_obs

1e313c4

update comments on 2a_model.R

cf6f251

galengorski force-pushed the med_obs_model branch from 0ea015e to cf6f251 Compare July 1, 2022 14:02

Galen Gorski and others added 3 commits July 1, 2022 11:24

change comments

5f6a821

changed input x_vars in config and site set in visualize.smk

18590ea

update snakemake call and x_vars

9ef4bac

galengorski requested a review from jsadler2 July 28, 2022 16:31

galengorski marked this pull request as ready for review July 28, 2022 16:31

galengorski mentioned this pull request Jul 28, 2022

Run model with medium observed sites #122

Closed

galengorski requested a review from lekoenig July 28, 2022 17:14

lekoenig approved these changes Jul 28, 2022

View reviewed changes

lekoenig mentioned this pull request Jul 28, 2022

Add model instructions to README #146

Open

galengorski linked an issue Jul 28, 2022 that may be closed by this pull request

Run model with medium observed sites #122

Closed

reconcile all x_var in config and add comment in 2a_model

80a8b6c

jsadler2 suggested changes Aug 5, 2022

View reviewed changes

lekoenig linked an issue Aug 11, 2022 that may be closed by this pull request

Evaluate the utility of adding more sites #49

Closed

PR revisions requested by J. Sadler, run models with med_obs_sites up…

015e850

…date weights

galengorski requested a review from jsadler2 August 16, 2022 17:06

galengorski merged commit 13fa05f into USGS-R:main Aug 16, 2022

galengorski deleted the med_obs_model branch August 16, 2022 17:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Med obs model #139

Med obs model #139

galengorski commented Jun 17, 2022 •

edited

Loading

lekoenig left a comment

lekoenig Jul 28, 2022

galengorski Jul 28, 2022

lekoenig Jul 28, 2022

lekoenig commented Jul 28, 2022

jsadler2 left a comment

jsadler2 Aug 5, 2022

galengorski Aug 8, 2022

jsadler2 Aug 5, 2022

jsadler2 Aug 5, 2022

galengorski Aug 8, 2022

jsadler2 Aug 10, 2022

jsadler2 Aug 5, 2022

jsadler2 Aug 5, 2022

jsadler2 Aug 5, 2022

jsadler2 Aug 5, 2022

	"../../../out/"+site_set+"_io.zarr",
	f"../../../out/{site_set}_io.zarr",

	obs_file="../../../out/"+site_set+"_io.zarr",
	obs_file="../../../out/{site_set}_io.zarr",

Med obs model #139

Med obs model #139

Conversation

galengorski commented Jun 17, 2022 • edited Loading

lekoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lekoenig commented Jul 28, 2022

jsadler2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galengorski commented Jun 17, 2022 •

edited

Loading