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
SGVector cleanup. #2582
Comments
I forgot to mention that it is important to do this separately, both to ease the review and the cleanup process. That means that the best practice would be to send small and individual pull requests for each method. |
See the wiki for more motivation why we want to do this. |
@iglesias, I would like to work on this. Shall I start by moving dot to CMath? Should I do more changes in the first pull request? |
According to what I wrote above, moving dot to CMath sounds good :-) I think that doing that one alone in a first pull request is good. |
@iglesias I'll leave the functions related to linear algebra (we might want to move them to linalg). So I'll leave functions like add, sum, norm, add_scalar, product, vector_multiply etc for now while we are working on linalg NATIVE implementations. Should I meanwhile move functions like max, min, mean, arg_max, arg_min, max_abs to CMath? Don't know where to put linspace and sorting functions. |
Sure. In my opinion, max, min, argmax, argmin, and linspace make sense in CMath. Mean might make more sense in CStatistics. About sorting I am not sure, I leave it up to you :-) But there might be already some sorting implemented in CMath (iirc). |
I am a bit confused. As far as I know, the declaration and definition of a template should be kept in one single file, but if we take the case of SGVector, then it has a header file and a cpp file. How is this working out? |
We do a small trick. Look at the end of SGVector.cpp. There, we have defined the classes for which SGVector can be templated (which are basically primitive data types). |
There is |
If they are not used anywhere, I'd say we can drop them. We can for sure drop the ones in SGVector. The CMath one can stay. |
There are three functions related to sorting in SGVector:
So if we move Should I also move |
In my opinion, |
@iglesias would it be fine , if I moved range_fill to CMath ? |
@curiousguy13, it sounds good to me, both range_fill and range_fill_vector should go out of SGVector. Do something nice with them (perhaps, simplifying to only one method) and move to CMath. Keep in mind unit tests. Looking forward to see the pull request. |
@iglesias I think range_fill, zeros, these are perfect to be shifted to linalg instead. |
It sounds good!
|
I would like to work on this. Shall I start by moving add to CMath? |
You shouldn’t move to CMath but to linalg instead. However linalg already has add. But it would be good to remove add from SGVector. AFAIK CMath (now Math) should be dropped. |
This is for the method add() in SGVector. In file:
In file:
In file:
The So indirectly, even linalg's |
hmm, I think you are confusing things. |
Is there a method in linalg to add a dense vector to a sparse vector and/or add two sparse vectors? |
|
don't think there is atm. |
Remove all the methods producing clutter in
SGVector
. This includes (although it is not limited to) math operations likelinspace
,dot
, and other operations likefind
. The objective is to have anSGVector
as lean as possible.Systematically, the work to be done for each method reduced is:
SGVector
to another class, like for instance toCMath
or to whichever it makes sense the most.SGVector
, then refactor them (and move them). Otherwise, write some good unit tests.See this pull request to gain understanding of what this task consists of, #2579.
The text was updated successfully, but these errors were encountered: