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 length check (setting it) for sequences #1117

Merged
merged 10 commits into from
Aug 29, 2024

Conversation

p-snft
Copy link
Member

@p-snft p-snft commented Aug 26, 2024

The test of the 0th element of a sequence being None is replaced by a test function that also sets the length of _FakeSequence objects, making them iterable afterwards. This is replacing the old hidden magic of setting the size on read access (from solph v0.5.3) by explicit write access at the same points in the code. (Just once in the beginning instead of multiple times on each access.)

Fixes #1115

For now, sequences that are too long are allowed.
We might want to warn in these cases.
@p-snft p-snft self-assigned this Aug 26, 2024
@p-snft p-snft changed the base branch from dev to v0.5 August 26, 2024 12:10
@p-snft p-snft changed the title Add length check (seting it) for sequences Add length check (setting it) for sequences Aug 26, 2024
@p-snft p-snft requested a review from a team August 26, 2024 14:20
Copy link
Member

@uvchik uvchik left a comment

Choose a reason for hiding this comment

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

Thank you for your effort.


if isinstance(sequence, _FakeSequence):
sequence.size = length
return True
Copy link
Member

@uvchik uvchik Aug 26, 2024

Choose a reason for hiding this comment

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

I would check it differently:

if isinstance(sequence, _FakeSequence):
    if sequence.size is None:
        return True
    else: 
        if sequence.size == length:
            return True
        else:
            return False 

For me a sequence fits for any size, therefore, it is always TRUE, regardless of the checked length.

This behaviour will change if you set a length. From this moment on the size is set. If you now check the wrong length it is not valid any more.

I do not like the hidden setter. I would not expect the function to set the size of the sequence.
If you really want this hidden magic, I would prefer the following:

if isinstance(sequence, _FakeSequence):
    if sequence.size is None:
        sequence.size = length
        return True
    else: 
        if sequence.size == length:
            return True
        else:
            return False 

This will fail if I ask for a different length later. In your function the size will be updated, even if the check ask for different length.

Copy link
Member Author

@p-snft p-snft Aug 26, 2024

Choose a reason for hiding this comment

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

I understand what you are saying. Still, I tried to avoid this being a "hidden setter": According to the docstring, valid_sequence explicitly makes the argument a valid sequence of the given length. I did this to solve #1115: In your results, you might want to iterate over all values, real and fake sequences, in the same way.

I see three ways of doing this:

  1. Proceed as suggested in this PR.
  2. Rename the function valid_sequence to something like assure_valid_sequence to emphasize possible changes by the function.
  3. Make this even more explicit, e.g. by
    a. using the explicit setter (_Fakesequence.size = length) when needed, or by
    b. renaming valid_sequence to make_valid_sequence. This would then raise an Error instead of returning False.
  4. Completely remove the _FakeSequence and keep scalars. Then, cast on demand (to an np.array).

I'd opt to do something quick for the v0.5 release (so that the old behavior is maintained) and to remove the _FakeSequence in v0.6.

src/oemof/solph/_plumbing.py Show resolved Hide resolved
src/oemof/solph/_plumbing.py Show resolved Hide resolved
Copy link
Member

@uvchik uvchik left a comment

Choose a reason for hiding this comment

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

Thank you.

@p-snft p-snft merged commit e3010d4 into release/v0.5 Aug 29, 2024
11 checks passed
@p-snft p-snft deleted the fix/length-of-fake-sequences-unset branch August 29, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FakeSequences Not Iterating Correctly
2 participants