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.

Sign up for GitHub

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

Jump to bottom

Light refactor of models based on experience with Predictive, forecasting, and plans for time handling. #249

Closed
3 tasks done
damonbayer opened this issue Jul 8, 2024 · 3 comments · Fixed by #305
Closed
3 tasks done

Light refactor of models based on experience with Predictive, forecasting, and plans for time handling. #249

damonbayer opened this issue Jul 8, 2024 · 3 comments · Fixed by #305
Assignees
@AFg6K7h4fhy2
Labels
clean up Good code that could be better good first issue Good for newcomers
Milestone
🦆 Olorotitan

Comments

@damonbayer
Copy link
Collaborator

damonbayer commented Jul 8, 2024 •
edited
Loading

  • n_timepoints_to_simulate should be renamed. Maybe just n_datapoints
  • We should not pad data inside or outside of the model. Right now we pad and then slice when we could just do neither. Addressed in Forecasting Interface #241
  • There should not be a separate fork if observed_data is None in either model Addressed in Forecasting Interface #241
This was referenced Jul 8, 2024
Merged
Merged
@damonbayer damonbayer added clean up Good code that could be better good first issue Good for newcomers and removed development task labels Jul 12, 2024
@damonbayer damonbayer added this to the O sprint milestone Jul 19, 2024
@AFg6K7h4fhy2 AFg6K7h4fhy2 mentioned this issue Jul 24, 2024
Merged
@AFg6K7h4fhy2 AFg6K7h4fhy2 linked a pull request Jul 24, 2024 that will close this issue
Change Naming Of n_timepoints_to_simulate #305
Merged
@gvegayon
Copy link
Member

In the context of #304, I realized we have the arguments n_timepoints and duration for seeding processes and processes in general. Shouldn't all be named duration, @damonbayer @dylanhmorris?

@damonbayer
Copy link
Collaborator Author

Shouldn't all be named duration, @damonbayer @dylanhmorris?

In #156, we standardized on n_timepoints, but I think we should do something with time units for these arguments.

@dylanhmorris
Copy link
Collaborator

New issue to discuss/decide this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@AFg6K7h4fhy2 AFg6K7h4fhy2

Labels
clean up Good code that could be better good first issue Good for newcomers
Projects
None yet
Milestone
🦆 Olorotitan
Development

Successfully merging a pull request may close this issue.

Change Naming Of n_timepoints_to_simulate CDCgov/PyRenew
4 participants
@gvegayon @dylanhmorris @damonbayer @AFg6K7h4fhy2