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

Expose public facing Cython in libmda layer #3734

Closed
hmacdope opened this issue Jun 25, 2022 · 13 comments
Closed

Expose public facing Cython in libmda layer #3734

hmacdope opened this issue Jun 25, 2022 · 13 comments
Assignees
Labels
Component-Core CZI-performance performance track of CZIEOSS4 grant

Comments

@hmacdope
Copy link
Member

Is your feature request related to a problem?

As part of #3683, the libmda folder with its own __init__.pxd was introduced to provide a single place to expose public Cython APIs for interoperability. For example if external Cython code wishes to create a Timestep directly one can use:

from MDAnalysis.libmda cimport timestep

cdef ts = timestep.Timestep(...)

This is similar to the strategy used in Cython itself whereby all of the libc and libcpp imports live in one place:
For example

from libcpp cimport vector
from libcpp cimport ...

Describe the solution you'd like

As more modules are Cythonised as part of CZIEOSS4 Performance track changes, we should continue to add them to the public Cython API.

This however does require some decisions as to what is public from the Cython side and what should remain internal.

Alternately if people do not like this idea we should probably discuss it and now is likely a good time. :)

Describe alternatives you've considered

Have everything be private and internal to MDAnalysis.

@hmacdope hmacdope added Component-Core CZI-performance performance track of CZIEOSS4 grant labels Jun 25, 2022
@hmacdope hmacdope self-assigned this Jun 25, 2022
@orbeckst
Copy link
Member

  • Interoperability is good.
  • Given that there's an established pattern for this approach, libmda sounds sensible.
  • However, I feel we should be more explicit and name it libmdanalysis so that it is especially clear in other code which API is being used. (We use "mda" a lot internally but using it on external-facing code is not good branding — search for "mda" and see what you find, it ain't us ;-) )
  • In terms of what should go in there: presumably whatever is stable and can be sensibly used from the outside. What do you have in mind? Timestep, AtomGroup, Atom, Universe are really our core data structures. Which of these will be cythonized?

@hmacdope
Copy link
Member Author

hmacdope commented Jun 28, 2022

Thanks @orbeckst! I will open an issue a PR to fix the naming.

The things that could be exposed as you say have to be sensible and stable.

  • Timestep in its entirety
  • Atomgroup (this is somewhat complicated by the amount of metaclass-y stuff going on eg Transplants)
  • A fast topology lookup table
  • Universe (to be investigated)
  • Others TBA

This is just a rough first idea and is obviously will be subject to a bit of evolution as we go.

@hmacdope
Copy link
Member Author

hmacdope commented Nov 4, 2022

@MDAnalysis/coredevs with this in mind would people be open to exposing libdcd and libmdaxdr as part of our public facing cython library Would be a simple change but may advertise the libdcd and libmdaxdr are capable of being used standalone.

@orbeckst
Copy link
Member

orbeckst commented Nov 4, 2022

Yes, why not?

Do you envisage packaging them separately?

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2022

So something that came up in a conversation somewhere else - libmdanalysis is a suitably big name clash with lib that it has a high potential to make folks confused. Is there any way we can avoid this direct clash? Could we call it anything else or could we put libmdanalysis within lib at least?

@hmacdope
Copy link
Member Author

hmacdope commented Nov 4, 2022

We can move it within lib for sure. The aim was just one entry point for the cython if you want it so it doesn't really matter where it goes

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2022

We can move it within lib for sure. The aim was just one entry point for the cython if you want it so it doesn't really matter where it goes

Is there a potential issue with cyclic imports?

@hmacdope
Copy link
Member Author

hmacdope commented Nov 4, 2022 via email

@hmacdope
Copy link
Member Author

hmacdope commented Nov 6, 2022

Are people happy with the name libmdanaylsis moved inside lib or is this still too confusing. Any other possible name suggestions?

@orbeckst
Copy link
Member

orbeckst commented Nov 6, 2022

I’m fine with the name, just spelled correctly ;-).

@IAlibay
Copy link
Member

IAlibay commented Nov 6, 2022

I'm happy with lib.libmdanalysis if that makes sense to folks / doesn't cause weird import issues. I take view that folks that will use libmdanalysis will understand the codebase more than the newcomers who might get confused by the presence of two lib* directories.

@hmacdope
Copy link
Member Author

Now that docs have been added as of #3921 we can add the libdcd and libmdaxdr cython headers to libmdanalysis as part of #3888 and #3892 respectively.

@hmacdope
Copy link
Member Author

I think we can close this now with the note that folks add their public facing cython to libmdanalysis from now on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component-Core CZI-performance performance track of CZIEOSS4 grant
Projects
Development

No branches or pull requests

3 participants