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

Add support for 4D arrays #141

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Add support for 4D arrays #141

merged 1 commit into from
Aug 12, 2024

Conversation

jbfaden
Copy link
Contributor

@jbfaden jbfaden commented Aug 12, 2024

🗒️ Summary

This adds support for 4-index arrays.

⚙️ Test Data and/or Report

test4DDouble() of ArrayObjectTest tests this code.

♻️ Related Issues

@jbfaden jbfaden requested a review from a team as a code owner August 12, 2024 20:26
Copy link
Member

@nutjob4life nutjob4life left a comment

Choose a reason for hiding this comment

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

Hi @jbfaden! Thanks for the contribution.

I'd like to know more what's being accomplished here aside from the title. Is it possible to fill in the pull request template? All I see is the boilerplate text still.

@jbfaden
Copy link
Contributor Author

jbfaden commented Aug 12, 2024

This is the first time I've written a modification for someone else's code. Where is the pull request template?

I'm working with Jordan, and I see that the library supports 3-index arrays of data but not 4-index arrays. It now supports them as well.

@nutjob4life
Copy link
Member

@jbfaden oh, great! So happy to have your help. Thanks for the explanation. That helps me understand the purpose.

FYI, the template is this text that appears at the top of your pull request; see this screenshot:

Screenshot 2024-08-12 at 3 56 25 PM

If you click the ⋯ and choose "Edit", you can replace the text like Brief summary of changes if not sufficiently described by commit messages with something like This adds support for 4-index arrays; replace One of the following should be included here: with the output of mvn test, and so forth.

In any case, this looks fine, so I'm adding my stamp of approval 👍

@jbfaden
Copy link
Contributor Author

jbfaden commented Aug 12, 2024

Okay, thanks for pointing that out. Look forward to the next one!

@nutjob4life nutjob4life merged commit d5dcd42 into NASA-PDS:main Aug 12, 2024
1 check passed
@jordanpadams jordanpadams changed the title add support for 4-index arrays Add support for 4-index arrays Aug 13, 2024
@jordanpadams jordanpadams changed the title Add support for 4-index arrays Add support for 4D arrays Aug 13, 2024
@jordanpadams
Copy link
Member

@jbfaden do you have an example product that we can add to our validate regression tests to ensure we can check these? If you have one that should fail, that would be excellent as well!

@jbfaden
Copy link
Contributor Author

jbfaden commented Aug 13, 2024

Joe has one, which I assume I could make available to you. I'll ask him.

@nutjob4life
Copy link
Member

@jordanpadams sorry if I merged this too quickly—I was excited by this contribution 😊

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.

3 participants