-
Notifications
You must be signed in to change notification settings - Fork 8
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
Create an adaptor for the old api dies inputs #1879
Closed
Razvan1928
wants to merge
53
commits into
main
from
users/rnazare/Create-an-adaptor-for-old-API-for-Dies-Inputs
Closed
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
537f1dc
created new rendering module and update cycle
munteannatan 7bb7ea3
made storybook control
munteannatan 22ef63d
lint
munteannatan 471befc
Change files
munteannatan 31c5e02
working version
Razvan1928 7eb3bd3
updated api and made experimental option
munteannatan 2e3968b
kinda working
Razvan1928 0b50720
Merge branch 'main' into users/nmuntean/arrow-api
munteannatan de8668d
changed api and fixed tests
munteannatan ea20084
updated validation object
munteannatan 4bfc216
removed highlight table
munteannatan 6380a96
Merge branch 'main' into users/nmuntean/arrow-api
munteannatan 4eccbf9
Update wafer-map-convertor.ts
Razvan1928 23ae363
added tests, restructured code
Razvan1928 4717a57
Update wafer-map-convertor.spec.ts
Razvan1928 20dcd92
Update wafer-map-convertor.spec.ts
Razvan1928 1be7f62
Update wafer-map-convertor.spec.ts
Razvan1928 0440434
Update wafer-map-convertor.ts
Razvan1928 79fb492
deleted computeMaximumNumberOfTags method
Razvan1928 333adfb
made waferMapDies readonly
Razvan1928 d2236a4
renamed 'arrays' into 'column data'
Razvan1928 5395fb9
added generated data for tests
Razvan1928 9653c5b
Update sets.ts
Razvan1928 e7ff01e
updated tests and typos
munteannatan 122ea39
Merge branch 'main' into users/nmuntean/arrow-api
munteannatan e6051fe
many changes
Razvan1928 a557ee6
Update wafer-map-convertor.spec.ts
Razvan1928 4f6f175
Update wafer-map-convertor.ts
Razvan1928 bb6277a
some renamings
Razvan1928 3bed57a
deleted .toString from table
Razvan1928 eeda2ba
added column name and types tests
Razvan1928 df2836a
made wafermap converter class stateless
Razvan1928 a1f7326
renamed convertor into converter
Razvan1928 6008965
added WaferMapLayerData type and ran prettier eslint
Razvan1928 b810d0b
Update wafer-map-converter.spec.ts
Razvan1928 d44774e
Update wafer-map-converter.spec.ts
Razvan1928 943d089
Merge branch 'users/nmuntean/arrow-api' of https://github.com/ni/nimbβ¦
Razvan1928 8158681
Update wafer-map-converter.spec.ts
Razvan1928 82ceee7
Update wafer-map-converter.spec.ts
Razvan1928 7b63cf2
validator test changes
munteannatan 95de8af
spy names
munteannatan 4b8209e
peer dependency add
munteannatan 34897a7
Merge branch 'main' into users/nmuntean/arrow-api
munteannatan 652a613
update renovate
munteannatan 294f01f
updated validity control
munteannatan 53baa67
spacing
munteannatan 2914c04
updated api description
munteannatan b7ccf34
Update wafer-map-converter.ts
Razvan1928 4db94e3
lint and render strategy
munteannatan fc9a5b1
change file update
munteannatan c6a6605
Merge branch 'users/nmuntean/arrow-api' of https://github.com/ni/nimbβ¦
Razvan1928 8ca7cea
Merge branch 'main' of https://github.com/ni/nimble into users/rnazarβ¦
Razvan1928 e0ce11f
deleted not needed changes
Razvan1928 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
7 changes: 7 additions & 0 deletions
7
change/@ni-nimble-components-9a663b6a-ecb5-4698-bd2c-77cb3281255a.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"type": "minor", | ||
"comment": "New Wafer Map Component API. Introduced `diesTable` and two rendering strategies switched by this input", | ||
"packageName": "@ni/nimble-components", | ||
"email": "[email protected]", | ||
"dependentChangeType": "patch" | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
10 changes: 10 additions & 0 deletions
10
packages/nimble-components/src/utilities/tests/wafer-sets.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
export const expectedColIndex: number[] = [0, 1, 1, 1, 2]; | ||
|
||
export const expectedRowIndex: number[] = [1, 1, 0, 2, 1]; | ||
|
||
export const expectedValues: number[] = [ | ||
88.55000305175781, 90.54000091552734, 92.55999755859375, 95.56999969482422, | ||
85.76000213623047 | ||
]; | ||
|
||
export const expectedTags: string[][] = [['g'], ['k'], ['n'], ['s'], ['c']]; |
45 changes: 45 additions & 0 deletions
45
packages/nimble-components/src/wafer-map/modules/wafer-map-converter.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
import { Field, List, Table, Utf8, vectorFromArray } from 'apache-arrow'; | ||
import type { WaferMapDie, WaferMapLayerData } from '../types'; | ||
|
||
/** | ||
* This class is used to convert old wafer map data to new wafer map data | ||
*/ | ||
export class WaferMapConverter { | ||
public static toApacheTable(waferMapDies: WaferMapDie[]): Table { | ||
const layers = this.populateLayers(waferMapDies); | ||
|
||
const columnData = { | ||
colIndex: vectorFromArray(new Int32Array(layers.colIndex)), | ||
rowIndex: vectorFromArray(new Int32Array(layers.rowIndex)), | ||
value: vectorFromArray(new Float32Array(layers.values)), | ||
tags: vectorFromArray( | ||
layers.tags, | ||
new List<Utf8>(new Field<Utf8>('', new Utf8())) | ||
) | ||
}; | ||
|
||
const table = new Table(columnData); | ||
|
||
return table; | ||
} | ||
|
||
public static populateLayers( | ||
waferMapDies: WaferMapDie[] | ||
): WaferMapLayerData { | ||
const layers: WaferMapLayerData = { | ||
colIndex: [], | ||
rowIndex: [], | ||
values: [], | ||
tags: [] | ||
}; | ||
|
||
waferMapDies.forEach((die, index) => { | ||
layers.colIndex.push(die.x); | ||
layers.rowIndex.push(die.y); | ||
layers.values.push(parseFloat(die.value)); | ||
layers.tags[index] = die.tags ?? []; | ||
}); | ||
|
||
return layers; | ||
} | ||
} |
69 changes: 69 additions & 0 deletions
69
packages/nimble-components/src/wafer-map/tests/wafer-map-converter.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import { | ||
Field, | ||
List, | ||
Table, | ||
Utf8, | ||
vectorFromArray, | ||
Float32, | ||
Int32 | ||
} from 'apache-arrow'; | ||
import { WaferMapConverter } from '../modules/wafer-map-converter'; | ||
import type { WaferMapDie } from '../types'; | ||
import { generateWaferData } from './data-generator'; | ||
import { | ||
goodValueGenerator, | ||
highlightedValueGenerator | ||
} from './value-generator'; | ||
import { | ||
expectedTags, | ||
expectedValues, | ||
expectedRowIndex, | ||
expectedColIndex | ||
} from '../../utilities/tests/wafer-sets'; | ||
|
||
describe('WaferMap Converter', () => { | ||
const seed = 1; | ||
const waferMapDies: WaferMapDie[] = generateWaferData( | ||
2, | ||
goodValueGenerator(seed), | ||
highlightedValueGenerator(seed) | ||
); | ||
|
||
it('should populate the wafer layers', () => { | ||
const layers = WaferMapConverter.populateLayers(waferMapDies); | ||
expect(layers.colIndex).toEqual(expectedColIndex); | ||
expect(layers.rowIndex).toEqual(expectedRowIndex); | ||
expect(layers.values).toEqual( | ||
expectedValues.map(value => parseFloat(value.toFixed(2))) | ||
); | ||
expect(layers.tags).toEqual(expectedTags); | ||
}); | ||
|
||
it('should convert wafer map data and types to apache arrow table', () => { | ||
const table = WaferMapConverter.toApacheTable(waferMapDies); | ||
|
||
const computedTable = new Table({ | ||
colIndex: vectorFromArray(new Int32Array(expectedColIndex)), | ||
rowIndex: vectorFromArray(new Int32Array(expectedRowIndex)), | ||
value: vectorFromArray(new Float32Array(expectedValues)), | ||
tags: vectorFromArray( | ||
expectedTags, | ||
new List<Utf8>(new Field<Utf8>('', new Utf8())) | ||
) | ||
}); | ||
|
||
expect(table).toEqual(computedTable); | ||
|
||
const columnNames = table.schema.fields.map(field => field.name); | ||
expect(columnNames).toEqual(['colIndex', 'rowIndex', 'value', 'tags']); | ||
|
||
const columnTypes = table.schema.fields.map(field => (field.type as Field).toString()); | ||
const expectedTypes = [ | ||
new Int32(), | ||
new Int32(), | ||
new Float32(), | ||
new List<Utf8>(new Field<Utf8>('', new Utf8())) | ||
].map(type => type.toString()); | ||
expect(columnTypes).toEqual(expectedTypes); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, again, I think the lack of context for how this class will be used is problematic. It's hard to understand the utility of this functionality even being encapsulated in a separate class. For instance, if there was a single place where we intended for either of these "conversions" to occur, then why can we not just make these private methods on the
WaferMap
component class?Further, is there a use-case in mind for when
populateLayers
would be called outside of the context of thetoApacheTable
method? If not, then why is it public? These are the kinds of questions that are more easily answered when this implementation is accompanied with its usage in production.Ultimately, while I think smaller PRs are a good goal, there are limits before they become problematic. At worst, you would be adding 4 files to a different PR, two of which are extremely light. My preference would be to bundle this PR with another, where the APIs presented here are actually used.
@munteannatan @Razvan1928
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I discussed with @munteannatan and you are right, we will postpone the work on this task until the infrastructure will allow this API to be actually used. If we will continue on this pr or we will reuse the code in a different one will be decided later.