(issue 563): bypass cartesian_product_df() when reading a saved LArray object #756
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gdementen
reviewed
Apr 9, 2019
There was a problem hiding this comment.
FWIW, here is what I had done for the same issue
gdementen@0c226a1
alixdamman
force-pushed
the
master
branch
3 times, most recently
from
9264074 to
01669f2
Compare
gdementen
reviewed
May 21, 2019
larray/inout/pandas.py
Outdated
| else: | ||
| if sort_rows or sort_columns: | ||
| import warnings | ||
| warnings.warn('sort_rows and sort_columns are not used when cartesian_prod is set to False', stacklevel=2) |
There was a problem hiding this comment.
I think we can do better than this warning.
There are 3 options here:
- we want to support sort_rows and sort_columns in that case. We could use sort_axes, but I am not in favor of this option.
- we do not want to support it in the long run but want to avoid user code to break (in the case they load a hdf file saved with a new version of larray but the user code still uses sort_columns/rows). In that case we should use sort_axes and issue a deprecation warning with a clear upgrade path ("use sort_axes blahblah instead").
- we do not want to support it in the long run and accept the breakage for our users. In this case, the breakage should be as obvious as possible, i.e. we should throw an explicit error with some clear upgrade path.
I prefer option 3 (because it's easiest and I don't think many users used those arguments) but option 2 is fine with me too.
gdementen
reviewed
May 22, 2019
There was a problem hiding this comment.
Note that when I mentioned what I had done for the same issue, I did not expect you to scrap your code and use mine! It was just for your information, in case you could salvage something from it. Now I feel bad about letting you know about my code. It is my fault my code wasn't merged yet, so you shouldn't have to "pay" for that.
| @@ -221,6 +221,8 @@ Miscellaneous improvements | |||
| * renamed `a_min` and `a_max` arguments of :py:obj:`LArray.clip()` as `minval` and `maxval` respectively | |||
| and made them optional (closes :issue:`747`). | |||
|
|
|||
| * improved speed of :py:obj:`read_hdf()` function when reading a stored LArray object (closes :issue:`563`). | |||
There was a problem hiding this comment.
You should add that one must load and re-save their files to benefit for this !
…ect is an LArray object
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Jun 7, 2019
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Aug 5, 2019
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Aug 5, 2019
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Aug 26, 2019
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Sep 23, 2019
alixdamman
added a commit
to alixdamman/larray
that referenced
this pull request
Oct 7, 2019
alixdamman
added a commit
that referenced
this pull request
Oct 8, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.