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.

Sign up for GitHub

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

Jump to bottom

[MATLAB] Add tests about time precision preservation when converting MATLAB duration to arrow time32 and time64 #38652

Closed
leihou6116 opened this issue Nov 9, 2023 · 1 comment · Fixed by #38661
Closed

[MATLAB] Add tests about time precision preservation when converting MATLAB duration to arrow time32 and time64 #38652

leihou6116 opened this issue Nov 9, 2023 · 1 comment · Fixed by #38661
Assignees
@leihou6116
Labels
Component: MATLAB Type: enhancement
Milestone
15.0.0

Comments

@leihou6116
Copy link
Contributor

Describe the enhancement requested

The current tests for MATLAB interface arrow.array.Time32Array.fromMATLAB and arrow.array.Time64Array.fromMATLAB don't cover the preservation of time precision when TimeUnit is specified. The design might be changed in the future. If we add tests to cover the current behavior, the tests will be helpful to evaluate impact in the future.

An example of current behavior:

>> t = duration(0,0,1.5, 'Format', 'hh:mm:ss.SSS')
t = 
  duration
   00:00:01.500

>> arrow.array.Time32Array.fromMATLAB(t)
ans = 
  Time32Array with 1 element and 0 null values:
    00:00:02

Component(s)

MATLAB
@leihou6116
Copy link
Contributor Author

take

leihou6116 added a commit to mathworks/arrow that referenced this issue Nov 9, 2023
@leihou6116
Merge branch 'apacheGH-38652' of https://github.com/mathworks/arrow i…
966589a 
…nto apacheGH-38652
@github-actions github-actions bot mentioned this issue Nov 13, 2023
Merged
kevingurney pushed a commit that referenced this issue Nov 16, 2023
@leihou6116
GH-38652: [MATLAB] Add tests about time precision preservation when c…
1fd11d3 
…onverting MATLAB duration to `arrow.array.Time32Array` and `arrow.array.Time64Array` (#38661)

### Rationale for this change

The current conversion from MATLAB duration to `arrow.array.Time32Array` and `arrow.array.Time64Array` loses time precision, and there is no test to cover such limitation. It is best practice to have tests cover software design. In addition, such tests will be helpful to evaluate the impact in the future when we improve the design.  

### What changes are included in this PR?  

I mainly added three test cases for each of `arrow.array.Time32Array` and `arrow.array.Time64Array`.

- Updated the basic test case to verify both class and value. In the MATLAB interface tests, we would like to verify the value to make sure there is no precision loss. The basic test case will serve as a test example when people learn to write tests. Updating the basic test case will set a good example for contributors to learn. 
- Test the default value of "TimeUnit".
- Test the functionality of "TimeUnit".

### Are these changes tested?

No software change. The updated test files passed on my local machine.

### Are there any user-facing changes?

No
* Closes: #38652

Authored-by: Lei Hou <leihou@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
@kevingurney kevingurney added this to the 15.0.0 milestone Nov 16, 2023
dgreiss pushed a commit to dgreiss/arrow that referenced this issue Feb 19, 2024
@leihou6116 @dgreiss
apacheGH-38652: [MATLAB] Add tests about time precision preservation …
28fd84d 
…when converting MATLAB duration to `arrow.array.Time32Array` and `arrow.array.Time64Array` (apache#38661)

### Rationale for this change

The current conversion from MATLAB duration to `arrow.array.Time32Array` and `arrow.array.Time64Array` loses time precision, and there is no test to cover such limitation. It is best practice to have tests cover software design. In addition, such tests will be helpful to evaluate the impact in the future when we improve the design.  

### What changes are included in this PR?  

I mainly added three test cases for each of `arrow.array.Time32Array` and `arrow.array.Time64Array`.

- Updated the basic test case to verify both class and value. In the MATLAB interface tests, we would like to verify the value to make sure there is no precision loss. The basic test case will serve as a test example when people learn to write tests. Updating the basic test case will set a good example for contributors to learn. 
- Test the default value of "TimeUnit".
- Test the functionality of "TimeUnit".

### Are these changes tested?

No software change. The updated test files passed on my local machine.

### Are there any user-facing changes?

No
* Closes: apache#38652

Authored-by: Lei Hou <leihou@mathworks.com>
Signed-off-by: Kevin Gurney <kgurney@mathworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@leihou6116 leihou6116

Labels
Component: MATLAB Type: enhancement
Projects
Arrow MATLAB
Archived in project
Milestone
15.0.0
2 participants
@kevingurney @leihou6116