-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix typo #1227
fix typo #1227
Conversation
@@ -354,7 +354,7 @@ def mergeable(self, other, mergebool): | |||
if isinstance( | |||
other, | |||
( | |||
ak._v2.contents.emptyArray.EmptyArray, | |||
ak._v2.contents.emptyarray.EmptyArray, |
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.
or should it just be ak._v2.contents.EmptyArray
? Contents module already imports all the things. In this module, there are a number of these, but the others follow the lower-case spelling.
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.
There shouldn't be any lower camelCase module names at all.
Using
egrep -r '\b[a-z]+[A-Z]' src/awkward/_v2/**/*.py | egrep -v '\bto' | egrep -v '\bkMax' | egrep -v '\bkSlice'
I found the one you just found and another one in indexedarray.py (in addition to the original), though that's it; the rest of the regex matches are not instances of this copy-paste error.
src/awkward/_v2/_connect/numexpr.py:# def getArguments(names, local_dict=None, global_dict=None):
src/awkward/_v2/_connect/numexpr.py:# context = numexpr.necompiler.getContext(kwargs, frame_depth=1)
src/awkward/_v2/_connect/numexpr.py:# numexpr.necompiler._names_cache[expr_key] = numexpr.necompiler.getExprNames(
src/awkward/_v2/_connect/numexpr.py:# arguments = getArguments(names, local_dict, global_dict)
src/awkward/_v2/_connect/numexpr.py:# arguments = getArguments(names, local_dict)
src/awkward/_v2/_connect/numpy.py:# rowsA = len(A)
src/awkward/_v2/_connect/numpy.py:# colsA = first
src/awkward/_v2/_connect/numpy.py:# rowsB = len(B)
src/awkward/_v2/_connect/numpy.py:# colsB = first
src/awkward/_v2/_connect/numpy.py:# if colsA != rowsB:
src/awkward/_v2/contents/bitmaskedarray.py: ak._v2.contents.emptyArray.EmptyArray,
src/awkward/_v2/contents/indexedarray.py: ak._v2.contents.emptyArray.EmptyArray,
src/awkward/_v2/contents/listarray.py: next_content = asListOffsetArray64._content
src/awkward/_v2/highlevel.py:_dir_pattern = re.compile(r"^[a-zA-Z_]\w*$")
src/awkward/_v2/operations/convert/ak_from_json_schema.py: "`\\uXXXX`" escape sequences. If an `"enum"` is given, strings are represented
src/awkward/_v2/operations/convert/ak_from_json_schema.py: if schema.get("minItems") == schema.get("maxItems") != None: # noqa: E711
src/awkward/_v2/operations/convert/ak_from_json_schema.py: assert ak._v2._util.isint(schema.get("minItems"))
src/awkward/_v2/operations/convert/ak_from_json_schema.py: instructions.append(["FixedLengthList", schema.get("minItems")])
src/awkward/_v2/operations/convert/ak_from_json_schema.py: out = ak._v2.forms.RegularForm(content, size=schema.get("minItems"))
src/awkward/_v2/operations/convert/ak_to_buffers.py: Dask, but partition numbers can be emulated by prepending a fixed `"partN-"`
As for whether it should be ak._v2.contents.emptyarray.EmptyArray
or ak._v2.contents.EmptyArray
, I slightly prefer the latter, though I'd make it fit the context of the surrounding code, if relevant.
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.
OK, in that case my version is fine, right?
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.
Your version is fine, though there's another one just like this in indexedarray.py:
src/awkward/_v2/contents/indexedarray.py: ak._v2.contents.emptyArray.EmptyArray,
Since you reported the first one in unmaskedarray.py, I figure it's only a matter of time before you hit the one in indexedarray.py, too.
Meanwhile, I'm more worried about the build failing. You made a change that can't affect compilation, and yet every compilation is failing (in pybind11, which was recently updated...).
I'm wondering why all the tests are failing in the build. I'm checking out a fresh copy of the repo and rebuilding it myself. I'm also running the whole test suite on main: https://dev.azure.com/jpivarski/Scikit-HEP/_build/results?buildId=8717&view=results |
The main branch is okay. I wonder what... Aha! This PR is reverting the old pybind11. That's one thing I don't like about submodules; they don't version control the same way as other files. Since this is a tiny PR, would you mind if I just replace it with a new one that only makes your two intended changes? |
Certainly. On Thursday you van tell me when the difference was :) |
It's just this (no need to take time out of next Thursday's meeting for it): You had a local git repo of awkward with pybind11 in it from before the submodule was updated, and then even if you Thus, I don't like submodules, but they're still the best solution we have for C++ header-only dependencies, and they're not too bad if we hardly ever update them (which is the case). |
Replaced by #1228. |
Reopening just to see if I successfully updated submodules - do not merge. |
hm, can't reopen - never mind |
I could reopen, so that you can test. But updating a submodule (without simply re-cloning the whole repo) is an annoying process: it involves Catching accidental changes to submodules in a pre-commit could be a good idea, though they really do change rarely. Soon, we'll be dropping dlpack and will only have two of them: pybind11 and RapidJSON. |
Something like
(question) |
Apparently it's a "no". Closing anyway :) |
Maybe, but like I said, I have to look it up each time. (That would be true of anything I do less frequently than once a week.) Thinking about the pre-commit check to prevent accidental submodule reversions, which is the biggest issue: git sees the submodule as a detached head—I don't think it would be able to distinguish "less up-to-date" from "more up-to-date." It would only see "different." Then, having such a pre-commit could make it hard to legitimately update the submodule. Unless the issue happens more frequently, it might be better to not create a solution for it that might have other problems. Currently, it's still the case that I'm the "single point of merge bottlenecks," so I can just be more vigilant about checking for that when I do a review. (The reason this hasn't come up before is because previously, I was also the one doing most of the commits as well. Now that we've moved into a phase where there's multiple contributors, but I'm still reviewing everything, I have to be on the watch for stale submodules. We don't yet need to incorporate that into formal review checklists, though, because that would be something for the phase, with multiple reviewers.) |
No description provided.