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.

Sign up for GitHub

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

Jump to bottom

Fix joining labels with different number of names #107

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Fix joining labels with different number of names #107

merged 1 commit into from
Jan 27, 2023

Conversation

PicoCentauri
Copy link
Contributor

When try to join labels of different number of names like

property_labels_1 = Labels(
    ("structure", "prop_1"), np.vstack(2 * [np.arange(5)]).T
)
property_labels_2 = Labels(
    ("structure", "prop_2", "prop_3"), np.vstack(3 * [np.arange(5)]).T
)

the _join_labels label function raises an error because np.unique doesn't work with arrays of inhomogeneous shape. This issue is fixed in this PR.

@PicoCentauri PicoCentauri added bug Something isn't working Python-API Related to the Python bindings to "core" labels Jan 26, 2023
@github-actions
Copy link

github-actions bot commented Jan 26, 2023 •
edited
Loading

Here is a pre-built version of the code in this pull request: wheels.zip, you can install it locally by unzipping wheels.zip and using pip to install the file matching your system

@github-actions
Copy link

The documentation for this pull request is (or will soon be) available on readthedocs: https://equistore--107.org.readthedocs.build/en/107/

agoscinski
agoscinski reviewed Jan 26, 2023
View reviewed changes
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Looks good, but I really have problems to understand the join function in the tests. Maybe I am missing something simple, but I dont see it.

python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Show resolved Hide resolved
python/src/equistore/operations/join.py Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/tests/operations/join.py Outdated Show resolved Hide resolved
python/tests/operations/join.py Show resolved Hide resolved
python/tests/operations/join.py Show resolved Hide resolved
python/tests/operations/join.py Outdated Show resolved Hide resolved
agoscinski
agoscinski reviewed Jan 26, 2023
View reviewed changes
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

Thanks, changes help me a lot. For me, only case 3 of the join operation in the doc is unclear

python/src/equistore/operations/join.py Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/tests/operations/join.py Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/tests/operations/join.py Show resolved Hide resolved
@PicoCentauri PicoCentauri force-pushed the fix_join_labels branch 2 times, most recently from 3283a13 to 391acb1 Compare January 27, 2023 09:29
@PicoCentauri
Copy link
Contributor Author

I hope I addressed all your comments and already squash the commits.

Luthaf
Luthaf reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@Luthaf Luthaf left a comment

Choose a reason for hiding this comment

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

Some small suggestions for the doc, the code & tests look good to me

python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
python/src/equistore/operations/join.py Outdated Show resolved Hide resolved
agoscinski
agoscinski reviewed Jan 27, 2023
View reviewed changes
Copy link
Contributor

@agoscinski agoscinski left a comment

Choose a reason for hiding this comment

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

I still dont really understand case 3 of the join operation, but I am okay to do this in a separate PR, might be good to add examples in this separate PR that make it clear. From my side approve.

@PicoCentauri PicoCentauri mentioned this pull request Jan 27, 2023
Open
@PicoCentauri
Copy link
Contributor Author

We can squash and merge if everybody is okay...

@PicoCentauri @agoscinski @Luthaf
Fix joining labels with different number of names
1a7f764 
Co-authored-by: agoscinski <agoscinski@users.noreply.github.com>
@Luthaf
Copy link
Member

Luthaf commented Jan 27, 2023

I did a tiny bit more reformatting of the docstring, I'll merge once CI is happy!

@Luthaf Luthaf merged commit 7ec266d into master Jan 27, 2023
@Luthaf Luthaf deleted the fix_join_labels branch January 27, 2023 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Luthaf Luthaf Luthaf left review comments

@agoscinski agoscinski agoscinski left review comments

Assignees
No one assigned
Labels
bug Something isn't working Python-API Related to the Python bindings to "core"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@PicoCentauri @Luthaf @agoscinski