kidusasfaw / spatPomp

R package for statistical inference of spatiotemporal partially observed Markov processes
GNU General Public License v3.0
11 stars 10 forks source link

Created listie objects for ibpf and bpfilter #16

Closed jeswheel closed 1 year ago

jeswheel commented 1 year ago

This change was prompted by a bug that arose in my Haiti cholera analysis when re-running the code after the new listie updates in the pomp package. The issue arises when running code like the following:

foreach(..., .combine = c) %dopar% ibpf(...) Because ibpfd_spatPomp inherits from spatPomp, which inherits from pomp, the output of the code is a pompList object. Because of this, all elements of the list are changed from spatPomp objects to pomp objects. In my code, this caused an issue because the pomp objects are missing slots for iterated objects, in particular they don't have a traces slot.

It seemed like the easiest fix was to simply make listie objects for spatPomps, ibpfd_spatPomps, and bpfilterd_spatPomps. To do this, I used the pomp listie code as a template. I then created / modified unit tests so that the package maintained it's current test coverage.

This change can also be easily extended to other spatPomp objects, like girfd_spatPomp.

Note: One possible improvement here is that because the conc generic from pomp is a non-exported function, I needed to copy this function into the spatPomp package and make the same generic function again rather than just adding new methods to this generic. If conc from pomp is exported then this duplication won't be necessary.

jeswheel commented 1 year ago

I'm not sure why the windows-latest (release) check failed. It got a HTTP response: 404 error message when setting up the environment, so I think it's a GitHub actions issue. This didn't happen when I pushed the commit to my fork just a few minutes before creating the pull request.

ionides commented 1 year ago

This pull request is timely - I recently noticed similar issues and added them to the todo list. I can't see why the Windows check is anything other than a problem with the Windows server, since there don't seem to be any Windows related issues in the changes. I'll re-run the tests.

kingaa commented 1 year ago

Jessie, though conc is not exported, there is the concat method, which is exported by pomp. Will this work for your purposes?

jeswheel commented 1 year ago

Jessie, though conc is not exported, there is the concat method, which is exported by pomp. Will this work for your purposes?

Thanks for point that out. The concat function calls the conc generic, so unless that is exported then there can't be conc methods for SpatPomp listie objects. However, I don't think recreating the conc generic is too big of an issue since it's a simple enough of a generic function, so it may not be necessary to export conc if it's preferred to keep this as a non-exported function.

One thing that your comment made me think of however is that a better improvement would have not re-made the concat function, and to make the SpatPomp class inherit from pomp, then the pomp::concat function should work for this package. I can work on this sometime in the near future.