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

Restructure models to use a "version" argument instead of separate functions #178

Merged
merged 23 commits into from
Mar 26, 2024

Conversation

venpopov
Copy link
Owner

Summary

This is the follow up to #177. Closes #163

the sdmSimple() model is now sdm()
the IMMfull(), IMMabc() and IMMbsc() are now all imm(), with a version argument:

  • imm(version = "full")
  • imm(version = "abc")
  • imm(version = "bsc")

the "full" is the default

I decided to leave the two mixture models separately. For them almost all the meta information is different - the model name, the citation, etc. So I thought it's easier to keep the documentation separately, otherwise the help page becomes complicated.

This framework should now work for future models that have multiple versions.

Tests

[X] Confirm that all tests passed
[X] Confirm that devtools::check() produces no errors

I reran all the internal reference fits tests as this is a more substantial change. All is good and we replicate previous fits.

Release notes

@venpopov venpopov changed the title Feature/model versions restructure Restructure models to use a "version" argument instead of separate functions Mar 26, 2024
- Adapt the  `use_model_template` function to the new `version` argument for `bmmodels`

- Minor change to the `bmmformula` function to remove redundancies in model specific `bmf2bf` functions
Copy link
Collaborator

@GidonFrischkorn GidonFrischkorn left a comment

Choose a reason for hiding this comment

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

I found some minor things in the model_imm.R script that I commented. Apart from that I did not run into any issues on my machine and like the cleaner setup for the different models. I also think this will make it easier to accomodate different task versions for these models (e.g. for change detection versus continous reproduction).

Thanks for taking care of this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we rename this file to test-bmm.R ? I think this would just be for consistency in naming, but it might still be good to just take care of this now.

R/model_IMM.R Show resolved Hide resolved
R/model_IMM.R Show resolved Hide resolved
@GidonFrischkorn
Copy link
Collaborator

In b3e85cb I added the changes to the use_model_template. You might want to check these changes and look if there is something missing.

R/model_IMM.R Show resolved Hide resolved
@venpopov
Copy link
Owner Author

In b3e85cb I added the changes to the use_model_template. You might want to check these changes and look if there is something missing.

Looks good! Thanks!

@venpopov venpopov merged commit 48adf14 into develop Mar 26, 2024
3 checks passed
@venpopov venpopov deleted the feature/model-versions-restructure branch March 26, 2024 21:04
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.

conventions on variables, arguments, functions, etc
2 participants