-
Notifications
You must be signed in to change notification settings - Fork 157
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
Add semantic dimension identifier to concat API #1244
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1244 +/- ##
==========================================
+ Coverage 84.27% 84.34% +0.06%
==========================================
Files 35 35
Lines 5693 5685 -8
==========================================
- Hits 4798 4795 -3
+ Misses 895 890 -5
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented on the original issue, but basically would like to discuss this.
To me, you are proposing that we do a broad API change of introducing a keyword argument dim
which is analogous to axis
, but refers to the axes as "obs", "var"
instead of 0, 1
. I would like to at least discuss the implications of this, especially considering that we allow names to be set dynamically on the dimensions. Let's do this on the issue.
I also would not consider this a bug fix as it introduces a new element to the API.
Co-authored-by: Ilan Gold <[email protected]>
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple points.
It would also be good to address the missing coverage on the added code.
axis_size
may be able to just get deleteddim_len
could probably get deleted (was it used somewhere?), or it should get a deprecation test
Co-authored-by: Isaac Virshup <[email protected]>
all done, including the |
@flying-sheep could you merge main on this one? |
done! |
Follows the consensus
axis
/axis_name