Description

sneakers-the-rat

opened on Apr 17, 2023

Issue body actions

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:

python-graphblas/graphblas/core/operator/unary.py

Line 342 in 0eb490f

"erf", "erfc", "lgamma", "tgamma", "acos", "acosh", "asin", "asinh",

and then it looks like those have to be redeclared with some additional metadata in the tests, eg here:

python-graphblas/graphblas/tests/test_operator_types.py

Line 44 in 0eb490f

UNARY = {

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

1. eriknw commented on Apr 18, 2023

   

   eriknw

   on Apr 18, 2023

   Member

   More actions

   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 _initialize methods, which I think would be sufficient to close this issue.

   Reacted by Sultan Orazbayev
2. eriknw

   added

   hygieneImprove code quality and reduce maintenance overheadImprove code quality and reduce maintenance overhead

   on Apr 18, 2023
3. sneakers-the-rat commented on Apr 25, 2023

   

   sneakers-the-rat

   on Apr 25, 2023

   Author

   More actions

   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!
4. jim22k commented on Jul 5, 2023

   

   jim22k

   on Jul 5, 2023

   Member

   More actions

   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.

   Reacted by Sultan Orazbayev

Issue actions