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

[Question] What should the final API/interface be? #9

Open
steven-murray opened this issue May 27, 2021 · 1 comment
Open

[Question] What should the final API/interface be? #9

steven-murray opened this issue May 27, 2021 · 1 comment
Assignees

Comments

@steven-murray
Copy link
Contributor

I think this would be a good place to decide on what the final API for vis_cpu/gpu should be, and what their relationship should be.

  • Passing in array of freq?
  • What constraints on how much of a subset gpu is of cpu?
  • Polarization?
  • Analytic beams?
@steven-murray steven-murray self-assigned this May 27, 2021
@steven-murray
Copy link
Contributor Author

Calling in @AaronParsons, @philbull and @hughbg to discuss this.

It would be really neat if we could settle on an API before publishing a "v1.0.0" of vis_cpu. Of course, it can be updated in the future, but it would be good to have something that we think is pretty stable (for the core engine).

Here's my thoughts at the moment:

  1. Concerning frequencies: I have nothing against doing the frequency loop inside the core engine since that might help with some caching. However, I see two potential problems. One is that it makes it less obvious to the outsider that the vis_cpu algorithm is doing the frequency loop last -- to the user they're all just in there somewhere. But this can be documented. Secondly, having the ability to cache things is nice, but it then brings up questions of how much/what to cache which we have to deal with in the core engine rather than in the wrapper. Maybe this isn't a massive concern though.
  2. I think we should keep the implementations in the following way: the _cpu function should support all functionality. The _gpu should support a subset of the functionality (no extra functionality). The API should be such that they both have exactly the same possible inputs and defaults. However, inside the GPU function, where certain values to certain parameters aren't supported, it should raise a custom error (eg. NoGPUSupport) that can be caught easily by outside code. This means the flagship implementation can be a little more general and support some useful cases (like analytic beams for example), but that they can never diverge too far. This should be consolidated and maintained in tests.
  3. I would urge that we update the API a little in terms of parameter names in the core engine. Some of them are in a "short" notation that I think is less clear than longer names.
  4. I vote that we definitely put polarization (i.e. Reimplement polarization handling in vis_cpu #8 ) into the code, both in cpu and gpu.
  5. I'm personally fine with analytic beams for _cpu so long as we gracefully exit from GPU if they are passed.
  6. Something to think more in-depth about would be @hughbg's code to update the astropy coordinate calculations. In principle, this should just apply to the wrapper, which can pre-compute the coordinate matrices. However, I think one thing he does there is to have a clever way of caching those results to speed some things up. This might be problematic for GPU code (not sure), and is also a lot more code overhead to maintain. I think maybe it would be good if @hughbg could show us how much of a performance benefit there is to those calculations being cached, and we can decide from there?

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

1 participant