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

move python helpers #71

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

move python helpers #71

wants to merge 1 commit into from

Conversation

casperdcl
Copy link
Member

@casperdcl casperdcl added documentation Improvements or additions to documentation enhancement New feature or request labels Nov 13, 2024
@casperdcl casperdcl self-assigned this Nov 13, 2024
@KrisThielemans
Copy link
Contributor

ah ok. I guess I was thinking about having generate in tools, but now that I see this, it makes sense to me. I think (!) I'm happy with this. I'll put it on Discord to see if anyone else has any input.

Presumably this will need 0.3.0 as opposed to 0.2.2.

I'd be nice to move the C++ stuff as well before we tag though, but I haven't given that any thought really.

@@ -2,12 +2,11 @@
# Copyright (C) 2023-2024 University College London
#
# SPDX-License-Identifier: Apache-2.0

import sys

import petsird
Copy link
Member Author

Choose a reason for hiding this comment

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

btw I'm leaving this as an absolute import to make it easier for users to copy-paste

Copy link
Contributor

Choose a reason for hiding this comment

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

but wouldn't that apply to the line below as well?

Copy link
Member Author

@casperdcl casperdcl Nov 13, 2024

Choose a reason for hiding this comment

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

ish... depends on whether you expect users to do from petsird.helpers import ... (which sounds to me like a failure on yardl's part... isn't the point of yardl to not require boilerplate helpers?)

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't the point of yardl to not require boilerplate helpers?

yes, but some of these helpers are a bit too complicated to be able to be auto-generated. (How would you encode matrix multiplications of rigid transformations and coordinates in yardl? Seems very hard...)

Copy link
Member Author

Choose a reason for hiding this comment

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

Feel free to change as you see fit :)

@casperdcl
Copy link
Member Author

casperdcl commented Nov 13, 2024

having generate in tools

I don't know enough about this project's long-term direction to advise either way.

Copy link
Contributor

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

As long as you rename the PR to "move python helpers", I'm fine. Let's not merge yet though and wait for some more comments (if any).

@casperdcl casperdcl changed the title move helpers move python helpers Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

organisation of files (and usability of PyPI and conda-forge distributions)
3 participants