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

Updating utils from CAROT to CMonge #22

Merged
merged 7 commits into from
Nov 10, 2024
Merged

Updating utils from CAROT to CMonge #22

merged 7 commits into from
Nov 10, 2024

Conversation

DriessenA
Copy link
Collaborator

Adding generic utils from CAROT to CMonge

@DriessenA DriessenA requested a review from jannisborn November 8, 2024 16:38
Copy link
Collaborator

@jannisborn jannisborn left a comment

Choose a reason for hiding this comment

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

Why moving from utils to analysis, that's inducing an unnecessary breaking change.

If you have to move it, create a dummy import from the utils file to ensure backwards compatibility

@DriessenA
Copy link
Collaborator Author

Why moving from utils to analysis, that's inducing an unnecessary breaking change.

If you have to move it, create a dummy import from the utils file to ensure backwards compatibility

Soo if I add the typing of ConditionDataModule and ConditionalMongeTrainer etc, there are circular imports. This only breaks a few notebooks (mostly in the CARs) which I have already changes.

@jannisborn
Copy link
Collaborator

Alright but type hints are not enough of a reason to induce a breaking change in a pypi package. Let's use strings instead as recommended by PEP conventions https://peps.python.org/pep-0484/#forward-references

@DriessenA
Copy link
Collaborator Author

Alright but type hints are not enough of a reason to induce a breaking change in a pypi package. Let's use strings instead as recommended by PEP conventions https://peps.python.org/pep-0484/#forward-references

So I tried this, if I don't import the modules, I still get <module> not defined errors and if I do import the modules it still get's a circular import error.

@jannisborn jannisborn merged commit ae94830 into main Nov 10, 2024
1 check passed
@jannisborn jannisborn deleted the utils_update branch November 10, 2024 12:43
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