Multi-step conversion + documentation changes + bug fixes + small code changes #206

AmroAlJundi · 2022-10-17T07:52:06Z

Added multi-step conversion to the library. More specific changes are:

Converter no longer finds a conversion function, but a conversion chain of function-context pairs.
Added a cached version of Convert that returns the intermediate formats when a multi-step conversion occurs.
Convert functions now can take a vector of contexts rather than a single destination context. Note: a version that takes a single context still exists for simplicity.
Added a BFS implementation for multi-step search. The cost of conversion is simply the number of conversion steps. In the future, once conversions start having different costs, we can implement Dijkstra's algorithm instead.
Modified FunctionMatcherMixin to work with multi-step conversion.

Additional changes:

Fixed bug in CMake macro add_opt_library in header only.
Changed template parameters in Reorder and GraphMetric bases to AutoX to indicate users shouldn't touch them.
Documented Converter further.
Added typedefs in Converter to make code easier to read.
Documented IOBase.
Rephrased the definition of the inverse permutation generated by reordering.
Moved preprocessor parameter structs outside their preprocess classes.
Installation was missing installing utils.h
Updated tutorial 1 to use bases.
Added a page containing a list of available components (formats and functionalities).

…efinition.

… it from code docs.

docs/pages/getting_started/usage.md

docs/pages/tutorials/001_reordering.md

ardasener · 2022-10-26T14:27:18Z

docs/pages/tutorials/003_cuda_spmv.md

+```
+Every preprocessing algorithm in the library (reordering, partitioning, feature extraction, etc.) can be implemented for many format types. In this case, `RCMReorder`is only implemented for the `CSR` type. However, when we set the `convert_input` parameter (last parameter) to `true` in the call, this will allow function matching to take place. 
+
+Function matching takes the input formats to a preprocessing and the contexts the user passes to the function call, and, if the input format can't be used directly for the preprocessing (i.e., no function exists for the input's format type), it attempts to convert the input (using the passed contexts) to a format for which an implementation exists in the preprocessing. 


Maybe specifically mention what conversion via the context means. So in this case the CSR actually gets copied to the CPU.

Makes sense.

src/sparsebase/preprocess/preprocess.h

ardasener · 2022-10-26T14:38:38Z

src/sparsebase/utils/converter/converter.h

+    ConversionCondition;
+
+//! A single conversion step composed of a conversion function and a context to use for the function
+typedef std::tuple<ConversionFunction, context::Context *> ConversionStep;


Is there a particular reason this is not an std::pair. Is this where we would add weights?

No, there isn't really a reason. It can be a pair. I opted to associate the cost with an entire chain rather than a step, but now that I think about it it's more general to associate steps with costs. I'll add that.

That cost is currently not being used, but in the future, it might become useful.

ardasener · 2022-10-26T14:52:05Z

src/sparsebase/utils/converter/converter.h

+    * \return a vector of format with the first being the original format, the last being the target format,
+    * and the rest being intermediate formats. If a conversion is empty or false, only returns the original format.
+    */
+  static std::vector<format::Format*> ApplyConversionChain(const ConversionChain& chain,


So if move conversion is enabled, only the last format of this vector is valid. But if it's not, doesn't it contain duplicates?

It does. Technically, all the formats in the chain will be different representations of the same logical Format object. However, if users don't want these formats, they will just get deleted at the next step in the execution (Execute).

I see now that users might want to save memory by deleting a format as soon as it is not needed. Maybe we can replace the move options with such an option? I mean, if someone wants to move convert, they probably don't want the intermediate formats, right? What do you think?

Yeah that was my thought process as well

Makes sense. Let me add that.

Okay, so here's what I did:

For ApplyConversionChain, I replaced is_move_conversion with clear_intermediate.

is_move_conversion wasn't being used to begin with since at that point, the conversion functions were already decided by GetConversionChain.

clear_intermediate would delete intermediate formats after each step, keeping only the start format and destination format.

Replaced is_move_conversion with clear_intermediate in ApplyConversionSchema for the same reason as above.

In FunctionMatcher, added a clear_conversion parameter to CachedExecute. Here is what happens when it's true and when it's false:

clear_intermediate == true: if a chain size > 1, all the formats between the first and the last are deleted. It will only return the destination format. (In contrast, Execute doesn't return any formats)

clear_intermediate == false: will return all the destination format and all the intermediate formats.

In FunctionMatcher in Execute, the call to CachedExecute will always clear intermediate formats. Then inside Execute, the destination formats are also deleted.

Now, I want to retract my statement that "if someone wants to move convert they probably don't want the intermediate formats." That can actually be the case. Imagine move converting CSC->CSR. This will do CSC -> COO -> CSR. However, if move conversions are used, we will end up with three objects but only 6 arrays:
row_ptr (CSR and COO share this)
col_ptr (CSC)
col (CSR and COO share this)
vals (COO and CSR share this)
row (CSC)
vals (CSR and COO share this)
vals_csc (CSC)
(CSC vals and the other two vals are different because of how edges are sorted)

Which is what the "view" idea would've required. So I think clearing intermediate and moving should be separate ideas.

What do you think?

Ok looks good to me

ardasener

It's good. If you fix the small nitpicky things I requested and answer my questions I can approve.

# Conflicts: # docs/pages/getting_started/usage.md

…eps, and fixed some docs.

AmroAlJundi added priority: now Critical priority state: review needed type: feature Brand new functionality, features, workflows, endpoints, etc labels Oct 17, 2022

AmroAlJundi self-assigned this Oct 17, 2022

AmroAlJundi linked an issue Oct 17, 2022 that may be closed by this pull request

Add multi-step conversion to FunctionMatcherMixin #156

Closed

AmroAlJundi added type: fix Iterations on existing features or infrastructure. Optimizations, refactoring, etc. and removed type: feature Brand new functionality, features, workflows, endpoints, etc labels Oct 17, 2022

AmroAlJundi changed the title ~~Multi-step conversion~~ Multi-step conversion + documentation changes Oct 19, 2022

This was linked to issues Oct 19, 2022

Rephrase definition of ordering to be more clear in docs and code #183

Closed

Update reordering tutorial and reordering how-to and possibly explanation to use bases instead of creating objects #161

Closed

AmroAlJundi removed a link to an issue Oct 20, 2022

Update reordering tutorial and reordering how-to and possibly explanation to use bases instead of creating objects #161

Closed

AmroAlJundi linked an issue Oct 20, 2022 that may be closed by this pull request

Update reordering tutorial and reordering how-to and possibly explanation to use bases instead of creating objects #161

Closed

AmroAlJundi changed the title ~~Multi-step conversion + documentation changes~~ Multi-step conversion + documentation changes + bug fixes Oct 20, 2022

AmroAlJundi changed the title ~~Multi-step conversion + documentation changes + bug fixes~~ Multi-step conversion + documentation changes + bug fixes + small code changes Oct 20, 2022

This was linked to issues Oct 21, 2022

Tutorial on using CUDA preprocessing #154

Closed

Add pages to docs containing a list of available formats/preprocessings/conversions #192

Closed

AmroAlJundi added state: pending Taking action type: docs Related to documentation and information type: feature Brand new functionality, features, workflows, endpoints, etc and removed state: review needed labels Oct 22, 2022

AmroAlJundi force-pushed the feature/multi_step_conversion branch from e7655a1 to f389917 Compare October 22, 2022 11:26

AmroAlJundi added state: review needed and removed state: pending Taking action labels Oct 22, 2022

AmroAlJundi force-pushed the feature/multi_step_conversion branch 2 times, most recently from acb9635 to 18c8622 Compare October 24, 2022 09:24

AmroAlJundi added 5 commits October 24, 2022 17:09

Added multistep conversion with BFS

39bff3f

Fixed bug in add_opt_library in header only

db5f816

Added docs for convert_input and to preprocess bases

684e38b

Added typedefs to Converters for clarity and updated API docs

88e1bb9

Documented IOBase code and rephrased reordering inverse permutation d…

c0b5680

…efinition.

AmroAlJundi added 7 commits October 24, 2022 17:09

Added tests for CanConvert

ed7a7dd

Moved params structs outside preprocessors

3c7dbce

Updated adding reordering how-to guide

5576cb4

Installation wasn't installing utils.h

d5004a5

Updated tutorial-001 to use bases

5875378

Added a tutorial for GPU-CPU communication and function matching.

a3b40fe

Added page with list of available contents and failed to add links to…

821a3f5

… it from code docs.

AmroAlJundi force-pushed the feature/multi_step_conversion branch from 18c8622 to 821a3f5 Compare October 24, 2022 14:09

ardasener reviewed Oct 26, 2022

View reviewed changes

docs/pages/getting_started/usage.md Outdated Show resolved Hide resolved

ardasener reviewed Oct 26, 2022

View reviewed changes

docs/pages/tutorials/001_reordering.md Outdated Show resolved Hide resolved

ardasener reviewed Oct 26, 2022

View reviewed changes

src/sparsebase/preprocess/preprocess.h Outdated Show resolved Hide resolved

ardasener reviewed Oct 26, 2022

View reviewed changes

ardasener suggested changes Oct 26, 2022

View reviewed changes

ardasener added state: revision needed Requires additional work before next review and removed state: review needed labels Oct 26, 2022

AmroAlJundi added 2 commits October 26, 2022 20:28

Merge branch 'develop' into feature/multi_step_conversion

1c505ec

# Conflicts: # docs/pages/getting_started/usage.md

Added clear_intermediate option to conversion, costs to conversion st…

418ccf0

…eps, and fixed some docs.

AmroAlJundi requested a review from ardasener October 27, 2022 06:44

AmroAlJundi added state: review needed and removed state: revision needed Requires additional work before next review labels Oct 27, 2022

ardasener approved these changes Oct 27, 2022

View reviewed changes

AmroAlJundi merged commit cd8f202 into develop Oct 27, 2022

AmroAlJundi deleted the feature/multi_step_conversion branch October 27, 2022 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-step conversion + documentation changes + bug fixes + small code changes #206

Multi-step conversion + documentation changes + bug fixes + small code changes #206

AmroAlJundi commented Oct 17, 2022 •

edited

Loading

ardasener Oct 26, 2022

AmroAlJundi Oct 27, 2022

ardasener Oct 26, 2022

AmroAlJundi Oct 26, 2022

AmroAlJundi Oct 26, 2022

ardasener Oct 26, 2022

AmroAlJundi Oct 26, 2022

ardasener Oct 26, 2022 •

edited

Loading

AmroAlJundi Oct 26, 2022

AmroAlJundi Oct 27, 2022

ardasener Oct 27, 2022

ardasener left a comment

Multi-step conversion + documentation changes + bug fixes + small code changes #206

Multi-step conversion + documentation changes + bug fixes + small code changes #206

Conversation

AmroAlJundi commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardasener Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardasener left a comment

Choose a reason for hiding this comment

AmroAlJundi commented Oct 17, 2022 •

edited

Loading

ardasener Oct 26, 2022 •

edited

Loading