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 Aminmax.json #87

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JessicaSun1022
Copy link
Contributor

No description provided.

garrettzywang
garrettzywang previously approved these changes Jun 20, 2024
@garrettzywang garrettzywang self-requested a review June 20, 2024 15:38
@garrettzywang garrettzywang removed their request for review June 25, 2024 14:56
@sauravpandey123
Copy link
Contributor

sauravpandey123 commented Aug 6, 2024

Good job, this solution works well. But some improvements can be made:

  1. The value for "Dim" can be set to "None, NONE, etc" (different cases) but your operation only handles the case when "none" is inputted, as seen in the "contain_text" primitive. You can first convert the "Dim" into lowercase using "lower_upper_case_text" the primitive and then use this result for the "contain_text" primitive. That way, no matter what you enter (None, NONE, NoNE, etc), they will all be converted to "none", and thus, all these cases are handled at once.

  2. Please also fix the line passing through an operation like the following picture (from parse_integer to parse_integer_1). This will make your graph cleaner.

image

@JessicaSun1022
Copy link
Contributor Author

Updated.

@Vchen20
Copy link
Contributor

Vchen20 commented Sep 30, 2024

It looks good but the same line is still intersecting the corner of the parse_integer operation. Please fix that.
Screenshot 2024-09-29 214339

@JessicaSun1022
Copy link
Contributor Author

Updated

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