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

Updated sample/tutorial files #407

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Updated sample/tutorial files #407

merged 4 commits into from
Dec 4, 2024

Conversation

alsnhll
Copy link
Collaborator

@alsnhll alsnhll commented Nov 26, 2024

Describe your changes.

I copied changes over that were made in the flepimop_sample repository during testing.

I just made some super simple updates the example configs. There was a small error in the outcomes section of two. Updated some parameters to give more reasonable results. Added one with vaccination. Updated file for turning simulated data to fake ground truth data.

Does this pull request make any user interface changes? If so please describe.

No

What does your pull request address? Tag relevant issues.

NA

Tag relevant team members.

TimothyWillard and others added 4 commits November 8, 2024 11:40
Came from actually reviewing and using the docs with @emprzy which
highlighted some short comings.
11/8/2024 Sync GitBook From documentation-gitbook Into main
@TimothyWillard
Copy link
Contributor

Just so I understand, the contents of examples/tutorials is copied directly from https://github.com/HopkinsIDD/flepimop_sample? Have we considered just folding in that repo into this repo? Or at least make the syncing not manual with the use of submodules?

Unless this is important to get done ASAP I have a preference for merging this into dev instead of main.

@TimothyWillard TimothyWillard added documentation Relating to ReadMEs / gitbook / vignettes / etc. config Relating to configuration files or their framework. low priority Low priority. labels Nov 26, 2024
@alsnhll
Copy link
Collaborator Author

alsnhll commented Nov 27, 2024

Hi @TimothyWillard , thanks for reviewing! For workflow reasons whenever I make or test tutorial material I always do it from that sample repo, because we always run things from project repos, and having one for this purpose is very useful. But we purposefully decided to fold those tutorials into the core flepimop repo to make sure it stays up to date with code changes there and can be accessed along with that code.
I am fine with merging this into dev instead of main (though these are really trivial changes that have zero effect on any flepi function ... it's just example configs). It would also be amazing if this syncing could be automatic!

@TimothyWillard
Copy link
Contributor

TimothyWillard commented Dec 2, 2024

For workflow reasons whenever I make or test tutorial material I always do it from that sample repo, because we always run things from project repos, and having one for this purpose is very useful. But we purposefully decided to fold those tutorials into the core flepimop repo to make sure it stays up to date with code changes there and can be accessed along with that code.

I'm not entirely sure I follow, is there a known issue with setting the project path to a subdirectory of the flepi path? I think:

export PROJECT_PATH=$FLEPI_PATH/examples/tutorials

works and is close to what we do in the gempyor CLI tests on the dev branch: https://github.com/HopkinsIDD/flepiMoP/blob/2c73f388692ef2aefd02a8668aa8631f713eb022/flepimop/gempyor_pkg/tests/shared_cli/test_cli.py. Having duplicate sets of code/config under version control makes it difficult to keep these in sync.

I think for now let's just change the target branch to dev and we'll address further improvements after this release cycle.

Copy link
Collaborator

@anjalika-nande anjalika-nande left a comment

Choose a reason for hiding this comment

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

This seems fine to me. I don't have a strong preference between merging it initially in dev versus main so we can do whatever is cleanest!

Copy link
Contributor

@pearsonca pearsonca left a comment

Choose a reason for hiding this comment

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

Seems fine, but: are there any knock on changes to the partial configuration examples?

@saraloo
Copy link
Contributor

saraloo commented Dec 2, 2024

I don't think there would be any changes to the config parts you worked on. If they worked before they should still work fine. Makes sense to me to merge into dev.

@alsnhll alsnhll changed the base branch from main to dev December 3, 2024 18:10
@alsnhll
Copy link
Collaborator Author

alsnhll commented Dec 3, 2024

I changed the base branch to dev instead of main. I am fine with removing the flepimop_sample repo and just having this single location for the example code within flepimop repo, as long as we make sure the documentation is updated to match, as running things from this tutorials folder is how we want new users to get started

Copy link
Contributor

@TimothyWillard TimothyWillard left a comment

Choose a reason for hiding this comment

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

LGTM

@TimothyWillard TimothyWillard merged commit 7674af0 into dev Dec 4, 2024
2 checks passed
@TimothyWillard TimothyWillard deleted the update_examples branch December 4, 2024 16:33
TimothyWillard added a commit that referenced this pull request Dec 4, 2024
Correct mismatch between partial and full example configs introduced in
GH-407.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config Relating to configuration files or their framework. documentation Relating to ReadMEs / gitbook / vignettes / etc. low priority Low priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants