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

Use limited ABI to reduce number of required wheels #97

Closed
pkienzle opened this issue Nov 18, 2020 · 9 comments
Closed

Use limited ABI to reduce number of required wheels #97

pkienzle opened this issue Nov 18, 2020 · 9 comments

Comments

@pkienzle
Copy link
Member

No description provided.

@bmaranville
Copy link
Member

This has progressed, and now works except for the polymer extension (calc_g_zs_cex.c)
That extension is using a fixed version of the numpy API, and cannot be compiled as-is with py_limited_api.

@richardsheridan - it has been suggested that we consider numba as an alternative to the compiled extension. Would you be opposed to that?

@richardsheridan
Copy link
Contributor

Hi Brian, good to hear from you!

I think numba is a fine idea, I was already experimenting with it at the time on my private repo of self-consistent field theory code: sf_nscf.

You can lift as much code as you need or just use it as an inspiration for what functions to write. This was back before you were allowed allocate memory inside a jitted function, so they are a bit awkward looking, but profiling showed the jitted numba functions were just as fast or better than the c extensions... I just didn't want to ask Paul to make numba a dependency just for my little addition.

In that code, fastsum and compose are faster simply because I'm iterating naturally over a Fortran-ordered 2D array, I think. Some profiling might be in order to see if they still make a difference.

Just @ me if you have questions or want a code review or whatever.

@bmaranville
Copy link
Member

bmaranville commented Feb 8, 2021 via email

@richardsheridan
Copy link
Contributor

The tests are quite sensitive numerically, they might catch it even if you implement it correctly 🤷

@richardsheridan
Copy link
Contributor

For example, you might be able to re-enable the SCFCache test by setting atol around 1e-8 and disabling rtol. The function is pretty nonlinear so rtol can bite you for otherwise negligible changes.

@pkienzle
Copy link
Member Author

pkienzle commented Feb 8, 2021

@richardsheridan I didn't see fastsum being used anywhere so I didn't include it.

I didn't see any speed increase from compose when I timed it against your polymer test functions in tests/refl1d/test_polymer.py so I didn't include that either. Maybe it makes a difference in real world cases but not in the tests? Less code is better, so unless there is a significant benefit I prefer to leave it out.

I did some sensitivity testing on SCFCache, and yes, it is really non-linear. Is that because of this particular parameter set, or is that generally true? The optimizers are stepping on the order of 1e-6, but I noticed significant differences when stepping your function by 1e-15. I fear that the fitting results aren't going to be reliable.

I've posted my branch on PR #103

@richardsheridan
Copy link
Contributor

Hi @pkienzle! I left a code review on the PR, I don't know if you use those much. Anyway it was just minor points.

That test was created at a particularly nonlinear point of parameter space to challenge the cache walk algorithm. There is a point beyond which the layer is just a step function and the thing won't converge; I think it is up to the users to switch models at that point. The rest of the parameter space is more well behaved.

Which optimizers do you mean? I was using DREAM for my project. The other solvers seemed well behaved with the limited testing I did on them, but I never really challenged them.

@richardsheridan
Copy link
Contributor

Oh and I have no complaint about leaving compose out, especially if you have tested it and the difference is marginal.

@bmaranville
Copy link
Member

This has been implemented (after converting the polymer extension code to numba) with d9ae39e

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

No branches or pull requests

3 participants