Repository navigation
[pyos] Pop out constants from methods #433
Description
I am seeing that what look like constants that describe the various operations/etc. things imported from graphblas are defined within functions in some places, eg. here:
and then it looks like those have to be redeclared with some additional metadata in the tests, eg here:
I'm not quite sure what that additional metadata is doing, it looks like the tests there are testing whether the list defined within the test matches the modules imported within the different operators, so maybe defining twice is desired behavior, but it seems like regardless you might do yourself a favor and move all the places where some constant-like thing is defined ephemerally as in the example above to a place where you might reuse it or allow it to be inspected. It looks like other constant-like things like regexes to match different types are stored as class attributes, so that might be how you want to do it?
part of: pyOpenSci/software-submission#81
Activity
eriknw commentedon Apr 18, 2023
The current code and approach has served us well up to this point. We have had a multi-year effort to discover how we want operators to behave. For example, we want
gb.unary.exp(1)to automatically coerce an integer input to use the float operator instead of raise an exception. This code has been updated incrementally and is complicated. By effectively "doing it twice" by doing it differently in the tests serves as a good sanity/consistency check, which has proven helpful. The tests will also be super-helpful if we ever refactor how we do operators, which seems likely.I do appreciate the scrutiny you've given the operators. They're important, and they're worth making more understandable/maintainable. I agree with much of your input, and you've given a solid "shape" of how things should be after a refactor. We want things to be much more declarative and easier to modify info or behavior of a single operator at a time (such as improve a docstring). When we do this refactor, it will probably address handing multiple backends (cffi-based and not cffi-based) that may have different operators. I expect such a declarative approach will be similar to, but different than the data in the tests. So, I expect the tests to remain largely the same post-refactor, and we can always discard the tests if they are no longer useful and/or become too burdensome.
We could perhaps do a light refactor before this larger refactor where we name and make introspectable all of the hard-coded sets, lists, and dicts that appear within the
_initializemethods, which I think would be sufficient to close this issue.sneakers-the-rat commentedon Apr 25, 2023
Agreed :). And i am only not adding "of course y'all know the needs and structure of the package better than I do, and I am assuming that there are reasons for everything being the way it is" to all my issues here for brevity lol.
Feel free to close this or keep it open as a reminder, whatever is more useful!
jim22k commentedon Jul 5, 2023
Refactoring the operator classes is definitely high up on the list. It will be needed to support multiple backends, but will also give us a much cleaner separation between code we want to work with and code which is only there to parse the CFFI pieces.