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

Update the check for a jagged array during _writeParams #2051

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

opotowsky
Copy link
Member

@opotowsky opotowsky commented Jan 11, 2025

What is the change?

Scenario where jagged evaluates to False has a loophole (with py313 and numpy 2.2.1): single-element numpy arrays mixed into a list with non-array entries fails in _writeParams. (Edit: it wasn't a loophole...it's that I was previously using a very old version of numpy that allowed jagged arrays, and so the parameter that ended up as a jagged array slipped through the cracks.)

This updates the check for jagged to account for this scenario by editing _getShape.

Why is the change being made?

I was getting this failure sometimes while running cases (with py313 and numpy 2.2.1):

    JaggedArray(temp, paramDef.name) if jagged else np.array(temp)
                                                    ~~~~~~~~^^^^^^
ValueError: setting an array element with a sequence. The requested array has an inhomogeneous shape after 1 dimensions. The detected shape was (782,) + inhomogeneous part.

This is because the logic for determining whether something is a jagged array misses the scenario where single element numpy arrays are mixed with non array/list entries. The code in _getShape evaluates to (1,) for all these array entries AS WELL AS for int, float, or None, which means the jagged check evaluates to False. Then np.array(temp) fails with the above error. So I have the _getShape helper function return 1 for the latter scenario. Then the set for a list of elements mixed with single element arrays is {(1,), 1}, and causes jagged to be True.

Jagged or not jagged, that is the question

We can nix the above update and keep jagged as False by editing the temp list like so:

temp = [
    t.item() if isinstance(t, np.ndarray) and len(t) == 1 else t
    for t in temp
]

I'm just assuming we want jagged to be true, so went with the solution in the PR. Also, this edits the list after the fact. I don't care either way.


Checklist

  • The release notes have been updated if necessary.
  • The documentation is still up-to-date in the doc folder.
  • The dependencies are still up-to-date in pyproject.toml.

@opotowsky opotowsky added the bug Something is wrong: Highest Priority label Jan 11, 2025
@opotowsky opotowsky changed the title Update the check for a jagged array Update the check for a jagged array during _writeParams Jan 11, 2025
doc/release/0.5.rst Outdated Show resolved Hide resolved
@opotowsky
Copy link
Member Author

@john-science it doesn't return anything cause all it does is write things to the DB. Towards the bottom of the method you see this:
image

@john-science
Copy link
Member

john-science commented Jan 13, 2025

Okay, one option is to move this private function out to being a regular, private method: Database._getArrayShape:

def _getShape(arr: [np.ndarray, List, Tuple]):
"""Get the shape of a np.ndarray, list, or tuple."""
if isinstance(arr, np.ndarray):
return arr.shape
elif isinstance(arr, (list, tuple)):
return (len(arr),)
else:
return (1,)

And then add some unit tests.

class Database:

        @staticmethod
        def _getShape(arr: [np.ndarray, List, Tuple]):

doc/release/0.5.rst Outdated Show resolved Hide resolved
Copy link
Member

@john-science john-science left a comment

Choose a reason for hiding this comment

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

Nice catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong: Highest Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants