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

AD support #27

Merged
merged 43 commits into from
Jun 25, 2024
Merged

AD support #27

merged 43 commits into from
Jun 25, 2024

Conversation

perrydv
Copy link
Contributor

@perrydv perrydv commented Jun 22, 2024

Here is the PR with AD support for the nimbleEcology distributions. This includes:

  • For all distributions except Nmixture, the same nimbleFunctions should now work for AD.
  • For Nmixture, there is a matching set of dNmixtureAD_* nimbleFunctions.
  • Nmixture calculations are refactored to maximize code re-use between AD and non-AD versions.
  • New tests: test-AD and test-NmixtureADnoDerivs (for the dNmixtureAD_* cases when used not for derivatives)
  • roxygen and vignette are updated.
  • Removed len parameter from the *_oneObs distributions.
  • Other minor changes.

@weizhangstats
Copy link
Collaborator

@perrydv Just to let you know that all N-mixture models with AD supported are in the Nmixture-AD branch. You can see how to use the code there. Let me know you want me to do anything.

@dochvam
Copy link
Collaborator

dochvam commented Jun 22, 2024

@perrydv Looks like dNmixture's AD test is failing during a NIMBLE compilation step if you want to take a look. Is this related to Wei's comment about the two different branches?

@perrydv
Copy link
Contributor Author

perrydv commented Jun 23, 2024

@weizhangstats Thanks for the comment. Yes, sorry for not explaining. For the reasons commented in your PR, I wanted to work on the Nmixture AD support a bit more. I wanted to retain a way for non-AD users to get the automatic Nmin and Nmax determination, and I wanted to separate the numerical stability "max_index" from getting baked in arbitrarily on the first call. (It is still baked in, but with a heuristic choice rather than arbitrarily due to parameters involved in the first call). It was easiest for me to consolidate everything in this PR with the other distributions, having drawn on your branch where I could.

@dochvam I will look at the test failure. I think you fixed the yaml setup, thanks. The tests run on my machine, so I'm hoping it is some trivial issue to fix.

@perrydv
Copy link
Contributor Author

perrydv commented Jun 23, 2024

I don't know the actions and yaml setup very well. I noticed it was still installing nimble branch ADoak, but at this point it should use the released version of nimble, so I changed that. I also updated some tests.

I also realized that the dBetaBinom naming seemed inconsistent with the others, so I created dBetaBinom_v and dBetaBinom_s. There could also be a dBetaBinom_oneObs, but it would not be needed by the Nmixture cases so I didn't create it.

@perrydv
Copy link
Contributor Author

perrydv commented Jun 24, 2024

Status update: Various details are tweaked and fixed and tests pass on OS X and Windows.
Tests are not passing on Linux and this appears to be due to the testing setup. I tried tweaking the linux-specific bits of the yaml by imitating nimble's, but I wasn't sure of some details.
R CMD check --as-cran passes except for warnings about the html related to the .Rd files. This seems related to a discussion on r-sig-mac, and it looks like those warnings can be ignored.

So I think it is very nearly ready to go. It would be nice to get linux testing working. And it would be good to have review of code and documentation.

@paciorek
Copy link
Contributor

I'm no longer seeing the Linux tests in the GHA report.

@dochvam
Copy link
Collaborator

dochvam commented Jun 24, 2024

I do see the GHA checks on ubuntu in the report, looks like they're all passing!

@paciorek
Copy link
Contributor

Oh, I see. There are two separate actions, one for windows and one not, so one needs to toggle between them via links on the upper left, unlike with the GHA for nimble itself.

@dochvam dochvam merged commit 6f62c31 into master Jun 25, 2024
10 checks passed
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.

4 participants