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

🎨 Introduce ordered collections and simplify .mapped() #1390

Merged
merged 4 commits into from
Jan 7, 2024

Conversation

tests/test_collection.py Outdated Show resolved Hide resolved
@Koncopd
Copy link
Member

Koncopd commented Jan 7, 2024

I don't think removing "auto" is necessary, it is easier to check with "auto" first then to manually check variables. Also it is important to avoid any join if the variables are exactly the same for performance reasons.

@Koncopd
Copy link
Member

Koncopd commented Jan 7, 2024

I believe removing join_vars="auto" doesn't simplify anything, just requires a user to do additional actions manually instead.

@falexwolf
Copy link
Member Author

@falexwolf falexwolf changed the title ♻️ Simplify mapped() 🎨 Introduce ordered collections and simplify .mapped() Jan 7, 2024
Copy link

codecov bot commented Jan 7, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7af73d8) 92.85% compared to head (b0d6ea4) 92.60%.
Report is 10 commits behind head on main.

❗ Current head b0d6ea4 differs from pull request most recent head e02d70d. Consider uploading reports for the commit e02d70d to get more accurate results

Files Patch % Lines
lamindb/dev/_mapped_collection.py 50.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1390      +/-   ##
==========================================
- Coverage   92.85%   92.60%   -0.26%     
==========================================
  Files          44       44              
  Lines        4284     4311      +27     
==========================================
+ Hits         3978     3992      +14     
- Misses        306      319      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

github-actions bot commented Jan 7, 2024

@github-actions github-actions bot temporarily deployed to pull request January 7, 2024 20:34 Inactive
@falexwolf falexwolf merged commit d774be9 into main Jan 7, 2024
8 checks passed
@falexwolf falexwolf deleted the simplifymapped branch January 7, 2024 21:07
@github-actions github-actions bot temporarily deployed to pull request January 7, 2024 21:15 Inactive
assert ls_ds.join_vars == "outer"
assert len(ls_ds.var_joint) == 6
assert len(ls_ds[0]) == 2
assert len(ls_ds[0][0]) == 6
assert np.array_equal(ls_ds[1][0], np.array([4, 5, 8, 0, 0, 0]))
print(ls_ds[0][0])
Copy link
Member

Choose a reason for hiding this comment

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

Are those print statements a relict from making the tests work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and a slight worry that this output might be helpful in the future again. It took me a while to understand the test, and so I thought I'd leave these statements there for a little longer.

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.

3 participants