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

2024-08-07 update : adding matrix normalization func. and using AR1 in spatial func. #53

Conversation

cbernalz
Copy link
Collaborator

@cbernalz cbernalz commented Aug 7, 2024

This PR is regarding two issues : #52 (comment) and #50 (comment) .

@cbernalz cbernalz added this to the P sprint milestone Aug 7, 2024
@cbernalz cbernalz self-assigned this Aug 7, 2024
@cbernalz cbernalz marked this pull request as ready for review August 7, 2024 17:14
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

A few minor things. Looks generally very good.

tests/testthat/test_matrix_normalization.R Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
Copy link
Collaborator

@kaitejohnson kaitejohnson left a comment

Choose a reason for hiding this comment

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

Just one suggestion for an additional test of the normalization matrix function.

inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
tests/testthat/test_matrix_normalization.R Show resolved Hide resolved
Copy link
Collaborator

@kaitejohnson kaitejohnson left a comment

Choose a reason for hiding this comment

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

This is looking great! I just realized that we should also add the ar1 process call to construct_spatial_rt_process.stan.

spatial_deviation_t_i = (spatial_deviation_ar_coeff*spatial_deviation_t_i)

I think for that one, you will have to for z pass in the iid normal deviation (not with the separate sd). Think you can write as is and pass in 1 for the sd.

Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Two coments:

  1. Suggest changing init_bool to init_stat throughout and clarifying the documentation.
  2. When processes that take in standard normal vectors and transform them, we should use the built-in rngs to generate the standard normals, rather than custom ones.

inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
@kaitejohnson
Copy link
Collaborator

This is looking great! I just realized that we should also add the ar1 process call to construct_spatial_rt_process.stan.

spatial_deviation_t_i = (spatial_deviation_ar_coeff*spatial_deviation_t_i)

I think for that one, you will have to for z pass in the iid normal deviation (not with the separate sd). Think you can write as is and pass in 1 for the sd.

Update: ignore this comment. I misunderstood how we were doing this here.

Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

A few typos but very close @cbernalz

R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
R/generate_simulated_data.R Outdated Show resolved Hide resolved
inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/aux_site_process_rng.stan Outdated Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
inst/stan/functions/construct_aux_rt.stan Outdated Show resolved Hide resolved
@cbernalz cbernalz requested a review from dylanhmorris August 7, 2024 20:36
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Correcting my own mistake, apologies

R/generate_simulated_data.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @cbernalz!

@dylanhmorris dylanhmorris dismissed kaitejohnson’s stale review August 7, 2024 21:40

change request withdrawn, see #53 (comment)

@dylanhmorris dylanhmorris merged commit d4ba92c into spatial-main Aug 7, 2024
4 checks passed
@dylanhmorris dylanhmorris deleted the 52-adding-fixed-geometric-mean-marginal-variance-correlation-matrix-correction-function branch August 7, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants