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

Rework-API #50

Closed
wants to merge 17 commits into from
Closed

Rework-API #50

wants to merge 17 commits into from

Conversation

Joroks
Copy link
Collaborator

@Joroks Joroks commented Jun 26, 2024

I got a bit carried away and reworked almost the entire API . There are a lot of breaking changes but I hope that these will be worthwile.

Changes

  • I've replaced the Enums with Singletons
    • These Singletons get exported so LAMMPS.API.LMP_TYPE_VECTOR reduces to TYPE_VECTOR
    • All extract methods should now be typestable!
  • Most extracts now have the kwarg copy which is true by default. I've done this because the pointers we recieve from LAMMPS are not persistent. I don't think it's a good idea to hand the user an Array that can suddenly cause a Segfault - at least not by default.
  • Most extracts now return Vectors with length=1 instead of Scalar values, when setting copy=false, this allows the user to modify the underlying data. (This is partly because of type stability as well)
  • Computes/Fixes are allways of type Float64. With this assumption, gather/scatter! should now always detect incorrect types.
  • gather also uses the newly defined Singletons instead of Float64/Int32 for the type parameter
    • This improves consistency across functions and allows the function to return a Vector or a Matrix while preserving type stability
  • gather_bonds, gather_angles, gather_dihedrals, gather_impropers, decode_image_flags, encode_image_flags, extract_setting, and create_atoms, is_running are now implemented

Todo

  • unittests, docstrings, Descriptive error messages

It probably would have been better to split this up into multiple PR's 😅

@Joroks Joroks changed the title Rework Rework-API Jun 26, 2024
@vchuravy
Copy link
Member

Yeah this is a bit too large for me to review with the limited time I have right now.

I am not sure the enum to type approach is necessary. It forces a lot more specialization and constant propagation should help make it more type stable.

I do think including the expected element type on the signature is a good move (like we had for gather)

@Joroks
Copy link
Collaborator Author

Joroks commented Jun 26, 2024

Yeah I figured as much. If you want me to, I can prepare you a more focused PR without the unecessary extras.
I'll focus on only making the changes to the extract methods.

Type stability

Currently, there a 4 structs:

  • _LMP_DATATYPE_CONST
  • _LMP_STYLE_CONST
  • _LMP_TYPE_CONST
  • _LMP_VAR_CONST

With most of them carrying type information:

  • _LMP_DATATYPE_CONST directly defines the return type Vector|Matrix and Char|Int32|Int64|Float64
  • _LMP_TYPE_CONST defines the dimensinality Scalar|Vector|Matrix and the elment type Int32|Float64
  • _LMP_VAR_CONST defines the dimensionality Scalar|Vector|Matrix and the element type Char|Float64

The only enum not carrying type information is _LMP_STYLE_CONST as this just decides, if we're looking for global, atom or local data.

From what I've gathered, the only way we can guarantee type stability is if we dispatch on the values of the enums. In fact, that's the only reason to have _LMP_DATATYPE_CONST as a paramter, as it's already possible to always detect the datatypes thorugh other means.

I also Investigated a tiny bit using @code_typed and it seems to me, that constant propagation doesn't have any impact on type stability. Although I have to admit, that I don't fully understand the magic of the Compiler/Optimizer.

I'd suggest we switch _LMP_DATATYPE_CONST, _LMP_TYPE_CONST, and _LMP_VAR_CONST from enums to types while keeping _LMP_STYLE_CONST as an enum.

@Joroks
Copy link
Collaborator Author

Joroks commented Jun 27, 2024

Closing this in favor of #51.

@Joroks Joroks closed this Jun 27, 2024
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.

2 participants