Skip to content
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

Conversation

Razvan1928
Copy link
Contributor

@Razvan1928 Razvan1928 commented Feb 29, 2024

Pull Request

🤨 Rationale

We wanted to maintain the compatibility with the old api data

👩‍💻 Implementation

Created a class that gets the old api data as input and returns it as an apache arrow table.

🧪 Testing

With a few unit tests

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

* This class is used to convert old wafer map data to new wafer map data
*/
export class WaferMapConvertor {
public colIndex: number[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without seeing this being used as intended (in production), it's difficult to comment on whether this is a proper implementation. My initial reaction to seeing this class, however, is why is this stateful? Converter classes rarely, if ever, maintain state related to the values they are converting. They should be taking in inputs, and producing some output. The state of the converter should be relegated to formatting options that can be used for multiple conversion operations, and even then, those types of options are commonly passed in to the conversion method, allowing for the class to be static, which would be ideal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the class stateless and the methods static, should be fine now. About the sentence 'Without seeing this being used as intended...' for now I don't think I get give you a quick demo, at best this is the table logged in the console after the conversion in the storybook
image
the new wafer map rendering capability is not done yet :(

@Razvan1928 Razvan1928 requested a review from atmgrifter00 March 5, 2024 14:41
/**
* This class is used to convert old wafer map data to new wafer map data
*/
export class WaferMapConverter {
Copy link
Contributor

@atmgrifter00 atmgrifter00 Mar 5, 2024

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 the toApacheTable 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

Copy link
Contributor Author

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.

… into users/rnazare/Create-an-adaptor-for-old-API-for-Dies-Inputs
Base automatically changed from users/nmuntean/arrow-api to main March 7, 2024 21:02
@rajsite
Copy link
Member

rajsite commented Jul 2, 2024

@munteannatan I'm going to close this draft PR for now since it has no activity for a while. Feel free to re-open when relevant.

@rajsite rajsite closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants