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

Closes #1337 - Adding support to generic attach for categorical and segarray #1342

Merged
merged 3 commits into from
May 10, 2022

Conversation

joshmarshall1
Copy link
Contributor

@joshmarshall1 joshmarshall1 commented Apr 29, 2022

This ticket (closes #1337):
This ticket also closes #1284:

Added methods to RegistrationMsg.chpl for generic attach command which takes in an optional type or will attempt to infer the type if not passed. Passing in the optional type will skip directly to the portion of the method that compiles the response message. This should allow users to pass the type to increase performance.

I added a method to MultiTypeSymbolTable.chpl that checks existence of a given name and returns a bool result. I didn't see any other methods that returned bool for checking existence, only checkTable which returned void if the name exists, or error if it doesn't. This new method prevents me from having to use a try/catch block with checkTable or having to access the tab property of the SymTab class directly.

Also added were from_return_message methods to categorical.py and segarray.py so the return could be passed from the generic attach method in util.py and parsed correctly by the corresponding class.

The existing test cases for pdarray and Strings generic attach were expanded to include a given and an inferred type test. Two new tests were added, one for Categorical and one for SegArray, also each with a given and an inferred type.

Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

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

Logic all looks good. One thing that I would like to change would be using + for all delimiters instead of ;. I see why you are using a separate delimiter here (Categorical.categories being a Strings object). That should be easy enough to glue together to create the Strings object properly using Strings.from_response_msg().

arkouda/categorical.py Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
arkouda/categorical.py Outdated Show resolved Hide resolved
arkouda/segarray.py Outdated Show resolved Hide resolved
arkouda/util.py Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
Copy link
Contributor

@Ethan-DeBandi99 Ethan-DeBandi99 left a comment

Choose a reason for hiding this comment

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

Looks good

@mhmerrill
Copy link
Contributor

@joshmarshall1 there is a basic conflict on this PR now. I could fix it but I rather you did so I don't screw it up with the web editor ;-)

@joshmarshall1 joshmarshall1 force-pushed the 1337_generic_attach branch from 2eef634 to 5fc7f56 Compare May 6, 2022 19:18
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Couple gripes but overall the logic looks good!

arkouda/categorical.py Outdated Show resolved Hide resolved
arkouda/util.py Show resolved Hide resolved
arkouda/util.py Show resolved Hide resolved
src/RegistrationMsg.chpl Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
src/RegistrationMsg.chpl Show resolved Hide resolved
src/RegistrationMsg.chpl Outdated Show resolved Hide resolved
@joshmarshall1 joshmarshall1 requested a review from stress-tess May 6, 2022 20:19
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

Looks good!

@mhmerrill mhmerrill merged commit 4403f79 into Bears-R-Us:master May 10, 2022
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.

Add SegArray and Categorical support to Generic Attach Categorical Create From Return Message
4 participants