Clarify some utility functions (yt.funcs) with more_itertools
#2893
Conversation
This file contains 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
neutrinoceros
force-pushed
the
more-itt
branch
14 times, most recently
from
a724329
to
71b9b3f
Compare
neutrinoceros
added
the
api-consistency
neutrinoceros
changed the title
yt.funcs) with more_itertools
|
In general I like this; I was initially opposed but this is indeed cleaner. Will review this. Every time one of these refactors pops up it makes me long for the opportunity to deeply refactor the field system. |
|
Yeah I mean I started this as an exercise to learn about more_itertools but in the end I find it's a good (small) step forward towards the fields refactor. :) |
neutrinoceros
force-pushed
the
more-itt
branch
from
15eadfe
to
c088865
Compare
matthewturk
approved these changes
Sep 11, 2020
|
@yt-fido test this please |
|
Hm. The doc build seems to be idle (previous run timed out too). @Xarthisius, do you have any clue what's happening here ? |
|
No idea, here's the error: and |
|
I think it's due to |
|
@yt-fido test this please |
neutrinoceros
force-pushed
the
more-itt
branch
from
c102147
to
9317d23
Compare
3 tasks
neutrinoceros
force-pushed
the
more-itt
branch
from
9317d23
to
8adface
Compare
neutrinoceros
force-pushed
the
more-itt
branch
from
c16b4ab
to
ebf0243
Compare
munkm
reviewed
Dec 16, 2020
Comment on lines
99
to
+105
| def ensure_tuple(obj): | ||
| """ | ||
| This function ensures that *obj* is a tuple. Typically used to convert | ||
| This function ensures that *obj* is a tuple. Typically used to convert | ||
| scalar, list, or array arguments specified by a user in a context where | ||
| we assume a tuple internally | ||
| """ | ||
| if isinstance(obj, tuple): | ||
| return obj | ||
| elif isinstance(obj, (list, np.ndarray)): | ||
| return tuple(obj) | ||
| else: | ||
| return (obj,) | ||
| return tuple(always_iterable(obj)) |
There was a problem hiding this comment.
since we have this update of ensure_tuple() that does return tuple(always_iterable(obj)), is it worth adding back the ensure_list() functionality that acts as a sanitizer and and use that in the code for consistency?
There was a problem hiding this comment.
actually my plan was to get rid of both. As far as I recall, ensure_tuple is only ever used to sanitize Dataset.perdiodicity attributes within the code base, and what's more is that I think it's only ever done in the Boxlib frontend, so I would rather just get this one down that reintroduce its sibling. Does this sound fair to you ?
There was a problem hiding this comment.
(sorry about not replying to your initial comment on this btw, it slipped my mind so it seems.)
There was a problem hiding this comment.
(sorry about not replying to your initial comment on this btw, it slipped my mind so it seems.)
Not a problem! I just wanted to make sure it didn't get lost!
I would rather just get this one down that reintroduce its sibling. Does this sound fair to you ?
I think that's totally fine. 🙂 I was just curious for consistency reasons.
There was a problem hiding this comment.
I'll remove ensure_tuple in a followup PR !
This was referenced Dec 16, 2020
|
@neutrinoceros just a note--I should have been following this more closely, but |
|
Thanks for reporting back. I honestly had no idea the content of yt.funcs could be a dependence downstream. I am sorry about this breakage. It should be easier to manage transitional deprecations and removals in the future now (see https://github.com/yt-project/yt/pulls). |
What do you think would be a good solution to remedy this @jzuhone ? Should we add it back in with a deprecation warning? |
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.
PR Summary
Refactor some internals using
more_itertools(which is already a transitive dependency).remove
yt.funcs.ensure_listthis function has two use cases
ensure_listis bit clunky and requires maintenance on our part, the more powerfulmore_itertools.always_iterabledoes the job.lambda x: list(always_iterable(x))When we specifically need to iterate transparently over a arbitrary number of fields,
always_iterabledoesn't work out of the box since a single 2-tuple should be treated as a single field, but it's easy to configure, so I added a simpleyt.funcs.iter_fieldsfunction:rename
yt.funcs.iterableThis function's name is confusing because it doesn't indicate that it's really a checker that returns a boolean (
is_iterablewould be clearer in that sense). On top of this it is also wrong because it actually only checks thatlen(obj)is valid, while some iterables have no length (e.g. infinite generators).I renamed the existing function to
is_sequence.other associated renames
yt.funcs.validate_iterable->validate_sequenceyt.geometry.coordinates.coordinate_handler.validate_sequence_width->validate_sequence_widthrework
yt.funcs.ensure_tupleandyt.funcs.just_oneEach can be simplified as combination of two functions from
more_itertools.Additionally,
ensure_tupleis now only ever used to validateds.periodicity, so there is some overlapping with #2868 and the functionality could be moved to theDatasetclass directly.This PR refactors the one place that
ensure_tuplewas used differently, namelyyt.frontends.enzo_p.misc.nested_dict_get.I also fixed a bug (and added a test for it) in this function while I was at it.
PR Checklist
iterabletois_sequenceiter_fieldsfunctionensure_listcalls with eitheriter_fieldswhere an iterator is needed,list(iter_fields())where we actually need a list of fields, andlist(always_iterable())elsewhere.ensure_listensure_tuplecalls withtuple(always_iterable())(or update ensure_tuple to the same effect ?)just_oneblack --check yt/isort . --check --diffflake8 yt/flynt yt/ --fail-on-change --dry-run -e yt/extern