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

Dynamic allocation of logging toggles #125

Closed
wants to merge 11 commits into from

Conversation

AdrienLeGuillou
Copy link
Member

Hi @krivit,

I made this PoC implementation of the grow-able logging vectors you suggests in #123.

Currently it does the following:

  • make difftime, difftail, diffhead and diffto into DiffVect.
  • DiffVect are basically growable vectors with:
    • content a heap allocated array of ints
    • size the current count of elements
    • capacity available memory allocated
    • methods to create, free, append and convert to SEXP
  • I kept maxchanges for now but it's only used to stop if to many changes occurs

I implemented only what is necessary here and not all the features of a true "vector".
The initial capacity of 100 for each is purely for testing purposes.

This implementation seems to work as I can run EpiModelHIV simulations with it.

Does this interest you? If so, what should I improve in this (very simple) PoC?

@krivit
Copy link
Member

krivit commented Aug 28, 2024

Thanks! A few comments:

  • Use Calloc(), Realloc(), etc., respectively, since they provide R's error handling. (See https://cran.r-project.org/doc/manuals/r-release/R-exts.html#User_002dcontrolled-memory-1 .)
  • There should still be a maximum upper bound on the number of changes, and exceeding it should result in a meaningful error message directing the user to modify the appropriate control parameter. This is important, because otherwise cryptic error messages may result if the user, say, sets a duration that's too low.
  • Since this may be useful elsewhere, we might want to implement a generic version of this, for type is passed on initialisation. (See, e.g., inst/include/ergm_khash.h in ergm.) Alternatively, we could build on klib's kvec type: https://github.com/attractivechaos/klib/blob/master/kvec.h .
  • For the same reason, we might want to call it something else.

@AdrienLeGuillou
Copy link
Member Author

AdrienLeGuillou commented Aug 28, 2024

Thanks a lot for the comments!

I just updated with the R_*alloc functions as suggested in the provided link.

I made this bespoke implementation actually because I did not know if you wanted the full kvec implementation added to the project and wanted to check where the changes needed to happen quickly. But if you think it maybe be used elsewhere in the codebase that could be a worthwhile addition.

@AdrienLeGuillou
Copy link
Member Author

AdrienLeGuillou commented Aug 29, 2024

I made a new implementation with kvec. I included the header here for testing purposes but also made a PR to ergm (statnet/ergm#578) to add it to inst/include.

I modified the allocators in kvec.h to uses the ones from R.h instead of stdlib.h. Contrary to khash.h, no allocator macros are available.

Concerning the error message. Isn't the current error message for MCMCDyn_TOO_MANY_CHANGES enough? here

Note, I am terrible at naming things, so do not hesitate to request changes or reorganizations.

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.

2 participants