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

Rename overlapping_axes -> intersect_axes & make overlapping_axes instead the intersection of names #110

Merged
merged 4 commits into from
Sep 8, 2024

Conversation

cooljoseph1
Copy link
Contributor

Changes as described in #108.

Right now haliax.overlapping_axes returns the intersection of two axes specifications, and raises an error if two axes of the same name have different sizes. I contend that this function should be renamed haliax.intersect_axes for two reasons:

It is the direct counterpart of haliax.union_axes
It returns axes, not the axes' names. In particular, it raises an error if two axes are named the same but have different sizes. The word overlapping suggests that it could be used to get the names in common between two axes specs--such as if you want to check if you need to rename any of them. Throwing an error is strange behavior here.
Hence, I suggest that haliax.overlapping_axes be renamed to haliax.intersect_axes, and the function haliax.overlapping_axes be changed to return the intersection of the axes' names.

The main reason to not make this change is that it would break the API, and that might be reason enough to leave it as is.

Copy link
Member

@dlwh dlwh left a comment

Choose a reason for hiding this comment

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

thanks!

@dlwh dlwh merged commit 11689bd into stanford-crfm:main Sep 8, 2024
3 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