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

drawing utils #27

Merged
merged 39 commits into from
Dec 12, 2024
Merged

drawing utils #27

merged 39 commits into from
Dec 12, 2024

Conversation

fgrunewald
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Nice work, I do have a few comments though.
Besides those, consider which functions are helper functions (or even complete modules) that should not be user exposed, and prefix those with a _.
As I see it there are 2 that need to be user accessible: draw_molecule, and vsepr_layout

cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
@pckroon
Copy link
Collaborator

pckroon commented Oct 21, 2024

Oh, one more note, do you want tests for this?

cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing_utils.py Outdated Show resolved Hide resolved
cgsmiles/drawing_utils.py Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
@fgrunewald fgrunewald requested a review from pckroon December 3, 2024 16:46
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

Excellent work. Lots of small comments, as per usual for big PRs ;)

cgsmiles/__init__.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Show resolved Hide resolved
cgsmiles/drawing.py Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout_utils.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout_utils.py Outdated Show resolved Hide resolved
cgsmiles/linalg_functions.py Outdated Show resolved Hide resolved
cgsmiles/linalg_functions.py Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@pckroon pckroon mentioned this pull request Dec 4, 2024
@fgrunewald fgrunewald requested a review from pckroon December 11, 2024 12:31
Copy link
Collaborator

@pckroon pckroon left a comment

Choose a reason for hiding this comment

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

I have fewer and fewer comments, probably a good sign ;)

cgsmiles/drawing.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout.py Outdated Show resolved Hide resolved
cgsmiles/graph_layout_utils.py Outdated Show resolved Hide resolved
@fgrunewald fgrunewald requested a review from pckroon December 12, 2024 15:43
cgsmiles/drawing.py Outdated Show resolved Hide resolved
Co-authored-by: Peter C Kroon <[email protected]>
@fgrunewald fgrunewald merged commit 879ecc4 into master Dec 12, 2024
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