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

Fix variable scenario filtering, and availability TS multiplier #91

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

Conversation

ktehranchi
Copy link
Collaborator

Fixes:

  • Fixes scenario filtering of variables by Band ID. We could potentially apply this filter to other Enum classes, but I didn't want to unnecessarily filter out data unless we knew that behavior was needed:

    R2X/src/r2x/parser/plexos.py

    Lines 1249 to 1258 in ebac92c

    # Filtering Variables to select only the correct band-id which should be used
    # when accessing Variable data. We may also need to apply this filtering to
    # other Enum classes.
    variables = data.filter(pl.col("child_class_name") == ClassEnum.Variable.name)
    variables = variables.group_by("object_id", maintain_order=True).map_groups(
    lambda groupdf: groupdf.filter(pl.col("band") == pl.col("band").min())
    )
    data = data.filter(pl.col("child_class_name") != ClassEnum.Variable.name)
    data = pl.concat([data, variables])

  • One of the paths for parsing TS data was missing a * record['available']

@ktehranchi ktehranchi requested a review from pesap November 22, 2024 19:03
@pesap pesap requested a review from JensZack November 22, 2024 19:19
@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 73.94%. Comparing base (3caade5) to head (ceb5bcf).

Files with missing lines Patch % Lines
src/r2x/parser/plexos.py 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   73.98%   73.94%   -0.05%     
==========================================
  Files          37       37              
  Lines        3729     3734       +5     
==========================================
+ Hits         2759     2761       +2     
- Misses        970      973       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@JensZack
Copy link
Collaborator

This looks good to me, but we should probably add a test to cover this case. Do we have a dataset we can run that hits these changes?

@ktehranchi
Copy link
Collaborator Author

So we don't have a database that covers this test case. I don't have access to plexos to make one. One idea would be to use a more complicated public file like the caiso's deterministic plexos file as a test case. But r2x fails to convert this atm.

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