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

More recent octahedral embed that can handle carbenes #85

Merged
merged 4 commits into from
Nov 14, 2024

Conversation

alex-l-m
Copy link
Contributor

Updated octahedral-embed that can handle the new molecule class. Should be harmless to merge into the existing repo since it just adds functionality that the existing repo doesn't use.

@alex-l-m alex-l-m requested a review from a team as a code owner October 28, 2024 15:47
Copy link
Collaborator

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

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

Since there are now eight new *.mol2 files in /modules/octahedral_embed/, should the original four *.mol files in /modules be removed?

@alex-l-m
Copy link
Contributor Author

Yes, I didn't notice thee redundancy, but those original four can be deleted.

To double-check this, I looked at how octahedral_embed finds the mol2 files. It checks its own path variable, which is:

In [2]: from bluephos.modules.octahedral_embed import __path__

In [3]: print(__path__)
['/home/alexlm/repos/bluephos/bluephos/modules/octahedral_embed']

So only the mol2 files in that directory should be necessary. Which means you can delete the mol2 files in bluephos/modules.

@xiangchenjhu
Copy link
Collaborator

Yes, I didn't notice thee redundancy, but those original four can be deleted.

To double-check this, I looked at how octahedral_embed finds the mol2 files. It checks its own path variable, which is:

In [2]: from bluephos.modules.octahedral_embed import __path__

In [3]: print(__path__)
['/home/alexlm/repos/bluephos/bluephos/modules/octahedral_embed']

So only the mol2 files in that directory should be necessary. Which means you can delete the mol2 files in bluephos/modules.

Thanks for the clarification! Could you make this change directly in this PR to keep everything self-contained? Thanks!

@xiangchenjhu xiangchenjhu self-requested a review October 30, 2024 15:43
@alex-l-m
Copy link
Contributor Author

Yes, I made the change to the PR.

I also confirmed that I can run the pipeline with those files deleted, and it can run xTB without errors.

Copy link
Collaborator

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

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

Thanks

xiangchenjhu
xiangchenjhu previously approved these changes Oct 30, 2024
Copy link
Collaborator

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

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

Great! Thanks!

@xiangchenjhu
Copy link
Collaborator

@alex-l-m A formatting issue was detected during the lint check in /octahedral_embed/init.py. Please run ruff format . to correct it. Thanks!

@xiangchenjhu xiangchenjhu self-requested a review October 30, 2024 16:30
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need formatting this file (ruff format)

@xiangchenjhu xiangchenjhu self-requested a review October 31, 2024 15:57
Copy link
Collaborator

@xiangchenjhu xiangchenjhu left a comment

Choose a reason for hiding this comment

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

Looks good, Thanks!

@xiangchenjhu xiangchenjhu merged commit c50204d into ssec-jhu:main Nov 14, 2024
5 of 6 checks passed
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