Skip to content

Conversation

WingCode
Copy link

@WingCode WingCode commented Jun 1, 2024

closes #803

@CLAassistant
Copy link

CLAassistant commented Jun 1, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@IvanIsCoding IvanIsCoding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I know your code does not compile yet, but I think we can get started on dicussing how to tackle the breaking change on the Rust side

Comment on lines +81 to +82
periodic: bool,
with_positions: Option<F>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking API change on the Rust side. On the Python side, you can use optional arguments but on Rust there is not much we can do.

@jpacold
Copy link
Contributor

jpacold commented Jun 10, 2024

Hi @WingCode -- I noticed that this PR seems to only add with_positions, so I opened a separate draft PR just on the periodic part. Please let me know if you are planning to continue looking at this issue (I don't want to get in the way or duplicate your work).

@WingCode
Copy link
Author

Hi @jpacold , Thank you for asking. Yes, you can go right ahead with issue. I am not sure when I will be able to get back to this issue.

@IvanIsCoding
Copy link
Collaborator

Closing this as obsolete after #1213. Nevertheless, thanks for getting the work started!

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.

Add support to hexagonal_lattice_graph for periodcity and position coordinates
4 participants