Add 2D Autoencoder and GMM Integration on Siracusa Target#190
Add 2D Autoencoder and GMM Integration on Siracusa Target#190Aldrago98 wants to merge 30 commits into
Conversation
Aldrago98
changed the title
diaconuccalin
left a comment
diaconuccalin
left a comment
There was a problem hiding this comment.
Submitting the first third part of the review, I will finish going through the rest of the files as soon as possible.
There was a problem hiding this comment.
I think that these changes were made to adjust your local work env, and should not be pushed to main. Please revert them.
There was a problem hiding this comment.
The only changes to this file are blank lines. Please revert them completely.
| BatchNorm_fp32( | ||
| ${data_in}, ${scale}, ${bias}, ${mean}, ${variance}, | ||
| ${data_out}, ${batch_size}, ${channel_size}, ${window_size} | ||
| ${data_out}, ${batch_size}, ${channel_size}, ${window_size}, ${epsilon}, ${channels_first} |
There was a problem hiding this comment.
Let's add tests for this new feature (one test with channels_first 0, another with channels_first 1, and one more with non-defaul epsilon value)
There was a problem hiding this comment.
Let's revert all the changes in this file. It seems to me that it was a temporary fix to assign the n_cores to a new variable, which is not needed anymore, so no need for the new variable either.
| if engine is not None: | ||
| node.attrs["engine"] = engine.name | ||
| if hasattr(engine, "n_cores"): | ||
| node.attrs["n_cores"] = engine.n_cores |
There was a problem hiding this comment.
This is not a good approach IMO. The number of cores is not a node attribute (conceptually, the node attributes should follow the ones that exist in the real ONNX nodes). Plus, this issue of passing the information about the number of cores should already be solved, and the value should already exist in the operator representation, it's passed here. If this value doesn't get passed in your case, we should identify the root cause.
There was a problem hiding this comment.
Looking a little more into it, maybe you need to add NeurekaEngine in the list here.
| tilerModel.addTensorDimToModel(ctxt, tensorName) | ||
|
|
||
| for idx, shapeDim in enumerate(_buffer.shape): | ||
| shape = [_buffer.shape] if isinstance(_buffer.shape, int) else _buffer.shape |
There was a problem hiding this comment.
I think that we should identify the root cause for which this fix was needed and fix it in that location, rather than here (transforming there the shape in an enumerable, rather than here.
|
|
||
| schedule = TilingSchedule({}, {}, [], []) | ||
| repScheme = VariableReplacementScheme({}, {}) | ||
| inputLoadSchedule: List[Dict[str, HyperRectangle]] = [{}] |
There was a problem hiding this comment.
Why is this change needed?
| shape = cls._normalizedShape(buffer.shape) | ||
| outputLoadSchedule[0][addrName] = HyperRectangle((0,) * len(shape), shape) | ||
|
|
||
| schedule = TilingSchedule(inputBaseOffsets, outputBaseOffsets, inputLoadSchedule, outputLoadSchedule) |
There was a problem hiding this comment.
Same here, why is this needed? Why separate the input and output? And why change the schedule from an empty one?
| if inputShapes[1] == () or inputShapes[1] == []: | ||
| inputShapes[1] = (1,) | ||
|
|
||
| # Scalars and singletons should broadcast to the tensor operand, |
|
|
||
| def __init__(self, name: str = '', shape = [1], values = [0]): | ||
| super().__init__(name, shape, values) | ||
| # Some Neureka lowering paths inspect global constants before type inference |
There was a problem hiding this comment.
Which ones? This looks to me like an unstable patch that doesn't really solve the root cause (why they inspect before type inference?)
2 participants
The goal of this branch is to implement a 2D autoencoder model integrated with a Gaussian Mixture Model (GMM) on the Siracusa target with Neureka support.
The implementation was developed incrementally through the following steps:
Added
Changed
Fixed
PR Merge Checklist
develcommit and pointing todevel.CHANGELOG.mdfile has been updated.