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

Improved performance of numerical integration #8

Merged
merged 6 commits into from
Jan 19, 2024
Merged

Conversation

ellessenne
Copy link
Contributor

Hi Joshua,

We have been using {JointFPM} for a couple of projects and struggled a bit with the computational time when standardising over covariates other than the comparison of interest (e.g., a treatment).

I went back to the code and after some experimenting, I re-implemented the underlying numerical integration with nice results (see a formal comparison here). This PR adds this on top of the existing Romberg integration (which is still the default method here), as that method has a nice iterate until convergence feature (compared to Gaussian quadrature, where performance depends on how many integration points we use). I think Romberg is, possibly, a better default as it does not require the user picking the number of integration points, but I'd be happy to discuss this more if you'd like!

This PR doesn't recompile the documentation – for that, I encountered some issues with the latest {roxygen2} (7.3.0) and the import statements from {data.table} (I will open a separate issue for that).

Hope this helps and, as always, happy to chat more about this any time!

Alessandro

@entjos
Copy link
Owner

entjos commented Jan 19, 2024

Hi Alessandor,

it's nice to hear that you're using the package for some of your projects. The performance of the standardisation has also been a bit of a struggle for me. I think using Gaussian quadrature for that instead given the clear gain in performance sounds good to me! I think overall, it is nice to be able to chose between two integration methods in the prediction function. It's interesting to see that the Gaussian quadrature implementation is that much faster now, my previous implementation of that was quite slow. However, I think my implementation didn't make as nice use of vectorisation 😀

I just made some changes to the formatting of the code, added some comments, and added some unit tests.

That's a strange issue roxygen. Previously there were no errors due to that, but I guess that will be an easy fix. I'll have a look at it in the issue.

Thank you very much for your helpful pull-request!

Best,
Joshua

@entjos entjos added the enhancement New feature or request label Jan 19, 2024
@entjos entjos closed this Jan 19, 2024
@entjos entjos reopened this Jan 19, 2024
@entjos entjos merged commit f5e7f81 into entjos:main Jan 19, 2024
8 of 18 checks passed
@entjos entjos linked an issue Jan 19, 2024 that may be closed by this pull request
17 tasks
@ellessenne
Copy link
Contributor Author

Thanks for considering this PR @entjos – and once again, thanks very much for developing this package! 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Release JointFPM 1.2.0
2 participants