Follow-up from "Add a mechanism to inject faults in graph"

The following discussions from !420 (merged) should be addressed:

@pineapple started a discussion:

You should probably have the same default value as the Node constructor (= 0)

@pineapple started a discussion:

You should probably have the same default value as the Node constructor

 * This method inserts fault creating Nodes between each Producer and their children.
 * Each fault Node is given a number of bits to flip (based on the size of the
 * weigths). During the forward pass, the fault operator flips n bits and
 * returns the tensor.

Fixed in 84929ff2.

@pineapple started a discussion:

Should probably be the same description as the C++ function.

Why is it not the same name as the C++ function?

Fixed in b60888d4.

@pineapple started a discussion:

Same here

Fixed in b60888d4.

@pineapple started a discussion:

#include <cstdint>    // std::uint32_t
#include <memory>
#include <string>

Fixed in cc393613.

@pineapple started a discussion:

#include "aidge/graph/Node.hpp"
#include "aidge/operator/FixedNBitFlip.hpp"

Fixed in cc393613.

@pineapple started a discussion:

#include <cstddef>    // std::uint32_t
#include <memory>
#include <string>

#include "aidge/graph/Node.hpp"
#include "aidge/operator/NBitFlip.hpp"

Fixed in cc393613.

@pineapple started a discussion:
Not used in this file.

Fixed in cc393613.

@pineapple started a discussion:
```
FaultLocations weightNodes();
```
Trailing return type is not currently used in Aidge so I would change this declaration.

Fixed in 0b329fd2.

@pineapple started a discussion:

Should be a reference (even a const one I think)

Fixed in 2ff4194f.

@pineapple started a discussion:

        for (std::size_t i = 0; i < op->nbInputs(); ++i) {

Fixed in 0b329fd2.

@pineapple started a discussion:

Why not simply insert the location whenever possible? In the current state you are also testing each Node valididty for forwarding, which is not the purpose of your function.

You should instead have something like this:
            if (category == InputCategory::Param or category == InputCategory::OptionalParam) {
               if(node->getParent(i) != nullptr) {
                   faultLocations.insert(
                     std::make_pair(node->getParent(i), node));
               }
It would have the added bonus of shortening the code.

Fixed in dc9b2102

@pineapple started a discussion:

Overall, do not use int for indexes. Use std::size_t.

Fixed in 0b329fd2.

@pineapple started a discussion:

use the C++ std::size_t instead of the C style size_t. We avoid C type in Aidge when possible

Also in 0b329fd2.

@pineapple started a discussion:

You are using way too much the auto keyword when not needed, it becomes confusing. Here it was just a int

I am in favor of AAA (Almost Alaways Auto), read https://herbsutter.com/2015/01/14/reader-qa-auto-and-for-loop-index-variables/.

@pineapple started a discussion:

The variable should be a std::size_t

Also, shouldn't it be called cumulativeWeightIdx?

Cf. the answer to the "too much auto" comment just above.

@pineapple started a discussion:

    for (const auto& location : faultLocations) {
        const std::shared_ptr<Producer_Op> op = std::dynamic_pointer_cast<Producer_Op>(
            location.first->getOperator());
        const std::size_t size = op->getOutput(0)->size();
        total += size;
        weights.push_back(size);
    }

⚠️ no guarantee from getWeightsFaultsLocations this is a Producer Fixed in 02ff35ef

@pineapple started a discussion:
```
#define AIDGE_FAULTS_FAULTCLASS_H_
```
Bad define, should follow google style

Fixed in 0b329fd2.

@pineapple started a discussion:

This MR lacks any test of the added Operators and recipes
@pineapple started a discussion:

You should use the std::size_t type, not int for indices and sizes

Fixed in 0b329fd2.

@pineapple started a discussion:

Since getWeightsFaultsLocations provides children (from getNodes so not nullptr) and parents (that are different from nullptr because it was already checked) this assertion is not needed

Fixed by removing the assertion in 00139038.

@pineapple started a discussion:

Here again, getWeightsFaultsLocations has no built-in guarantee it is a Producer. You should add one or use std::dynamic_pointer_cast

Fixed in 02ff35ef.

@pineapple started a discussion:

You could even prepare the vector with .reserve(faultLocations.Size()) to avoid several consecutive reallocations in the following loop for the internal array of the vector.

Fixed in 7103097a.

@pineapple started a discussion:

The same log is a debug log (line 102). Maybe this one should be too
@pineapple started a discussion:

You should use GraphView::insertParent instead

Fixed in 565bb125.

@pineapple started a discussion:

You should use GraphView::insertParent instead

Fixed in 565bb125.

@pineapple started a discussion:

What is the purpose of this attribute? It not documented nor used anywhere.

Documentation is missing for the operator.
@pineapple started a discussion:

Maybe listing weights sizes for proportional distribution of bits to shift should be part of distributeFaults. It is written twice only to compute values used by distributeFaults.
@pineapple started a discussion:

Does it need to throw an error? It could even be a simple info log of getWeightsFaultsLocations.

Fixed in 8b6c6890.

@pineapple started a discussion:

The log might be an info or it will be printed each time. I am not sure, what do you think? Is it the expected behaviour?

Fixed in 8b6c6890 (notice to info log level).

@pineapple started a discussion:

Maybe you could add a warning or assertion here

Fixed in c5ba159f (adding a warning). This is also tracked here : #285 (closed)

Edited Jun 17, 2025 by Jerome Hue