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

Issue 53: First pass at differenced AR #128

Merged
merged 23 commits into from
Mar 15, 2024
Merged

Issue 53: First pass at differenced AR #128

merged 23 commits into from
Mar 15, 2024

Conversation

seabbs
Copy link
Collaborator

@seabbs seabbs commented Mar 8, 2024

This PR is ready for review and closes #53

@codecov-commenter

This comment was marked as outdated.

@seabbs
Copy link
Collaborator Author

seabbs commented Mar 8, 2024

Any feedback before I push on?

@SamuelBrand1
Copy link
Collaborator

I'm still parsing this

@seabbs
Copy link
Collaborator Author

seabbs commented Mar 11, 2024

This is ready for review.

I have added unit tests but given the blocker of #126 I think we need to wait for that for the documentation (which as noted is a serious issue).

I have also opened a new PR for the differencing (#134 which I will address after this) and made an issue for the lack of consistency for positional args (#133).

Copy link
Collaborator

@SamuelBrand1 SamuelBrand1 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 great; love the self-referencing approach to getting generic AR/diff AR!

I think the main AR struct needs to be parametric, thats the only change I recommend.

EpiAware/src/latentmodels/autoregressive.jl Outdated Show resolved Hide resolved
@seabbs seabbs enabled auto-merge March 12, 2024 20:16
@seabbs seabbs requested a review from SamuelBrand1 March 12, 2024 20:16
@seabbs
Copy link
Collaborator Author

seabbs commented Mar 12, 2024

I've updated w/ all suggestions.

Note I have used @with_kw from parameters.jl vs the base implementation as it allows for assets to be placed inside the inner constructor and so makes the implementation cleaner. I have also removed the specification of p as this doesn't work well when using defaults as it starts to conflict with the inner constructor.

@seabbs
Copy link
Collaborator Author

seabbs commented Mar 13, 2024

@SamuelBrand1 pinging for review

@SamuelBrand1
Copy link
Collaborator

@SamuelBrand1 pinging for review

Reviewing this now

Copy link
Collaborator

@SamuelBrand1 SamuelBrand1 left a comment

Choose a reason for hiding this comment

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

LGTM, only concern is that the method isn't robust to generating vector shaped damp or ar_init.

If this gets tested and works then I'm happy

EpiAware/test/test_autoregressive.jl Show resolved Hide resolved
@seabbs
Copy link
Collaborator Author

seabbs commented Mar 15, 2024

This should be updated now following the approach suggested in #134 and rebased on main. Note I had to copy docstringz templates everywhere due to submodules (a good downside we should have flagged) but I think the duplication isn't that bad as its something that gets touched rarely.

Moving on to #134 now.

@seabbs seabbs dismissed SamuelBrand1’s stale review March 15, 2024 09:46

all changes done

Copy link
Collaborator

@SamuelBrand1 SamuelBrand1 left a comment

Choose a reason for hiding this comment

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

I like this.

@seabbs seabbs merged commit 13398e0 into main Mar 15, 2024
10 checks passed
@seabbs seabbs deleted the issue53 branch March 15, 2024 11:10
@seabbs seabbs mentioned this pull request Mar 15, 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.

AR(1) latent process
3 participants