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

Reorganize the code base #904

Merged
merged 6 commits into from
Feb 4, 2025
Merged

Reorganize the code base #904

merged 6 commits into from
Feb 4, 2025

Conversation

lbittarello
Copy link
Member

This PR breaks some files into smaller ones. It doesn't introduce any substantive changes.

Functions that are imported by another file have been renamed so they don't start with a leading underscore.

Copy link
Collaborator

@stanmart stanmart left a comment

Choose a reason for hiding this comment

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

Thank you, I think the new structure is much nicer than what we had before. _glm.py in particular was rather unwieldy. I just have a few small comments, plus the following bigger ones.

Functions that are imported by another file have been renamed so they don't start with a leading underscore.

Where does that convention come from? I though we mostly use underscores to denote functions which are private for the users of glum. I understand that these functions live in underscored modules, which can mean the same thing, but why switch? I personally don't see a huge benefit in using names to denote whether functions are module-private or package-private. When working on glum, it's simple to use 'Go to referencesandRename symbol` to make changes across files anyways.

This is not to say that I'm totally opposed to the proposed change. If the consensus is to follow the new convention, that's fine with me.

This PR breaks some files into smaller ones. It doesn't introduce any substantive changes.

This is my more important point: we should still check downstream projects before merging this to make sure we don't break to much stuff the this refactor (or at least give them some time for them to prepare for it). I know that you are not changing the public API, but it would not surprise me if downstream users were relying on some private functions, too.

@lbittarello
Copy link
Member Author

Where does that convention come from? I though we mostly use underscores to denote functions which are private for the users of glum. I understand that these functions live in underscored modules [...], but why switch? I personally don't see a huge benefit in using names to denote whether functions are module-private or package-private.

At the moment, there's no pattern to which functions take a leading underscore. Some private functions don't have one (e.g., is_pos_semidef), some do. I tried to be more systematic. The advantage of this convention is that you can immediately tell whether other modules call a function or not when reading a file. We could also just use trailing underscores everywhere though.

Why not simply _glm_py, and the current _glm.py could become _glm_base.py. That would be a bit more aligned with class names IMO.

It would, but I'm trying to minimise downstream breakage.

@stanmart
Copy link
Collaborator

stanmart commented Jan 31, 2025

At the moment, there's no pattern to which functions take a leading underscore.

In that case I'm fine with either, and agree that we should pick one and be consistent.

@stanmart
Copy link
Collaborator

stanmart commented Jan 31, 2025

Could this solve the doctest issue?

Edit: ah, nevermind, I see you handled it already.

@lbittarello
Copy link
Member Author

Could this solve the doctest issue?

It didn't.

Copy link
Collaborator

@stanmart stanmart left a comment

Choose a reason for hiding this comment

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

LGTM but I imagine @jtilly and @MarcAntoineSchmidtQC have more to say

@lbittarello lbittarello merged commit 4f9db8f into main Feb 4, 2025
25 checks passed
@lbittarello lbittarello deleted the reorganize branch February 4, 2025 11:45
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