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

DiscreteVariable: Remove 'ordered' argument #5315

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

VesnaT
Copy link
Contributor

@VesnaT VesnaT commented Mar 8, 2021

Issue

Ordered is deprecated and should be removed from constructor.

Description of changes

Remove 'ordered' argument from DiscreteVariable's constructor.

Includes
  • Code changes
  • Tests
  • Documentation

@VesnaT VesnaT force-pushed the remove_ordered branch 2 times, most recently from 35f40fa to 4e7a965 Compare March 8, 2021 12:44
@codecov
Copy link

codecov bot commented Mar 8, 2021

Codecov Report

Merging #5315 (8b1f8d1) into master (9116768) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5315      +/-   ##
==========================================
- Coverage   85.35%   85.35%   -0.01%     
==========================================
  Files         301      301              
  Lines       62038    62035       -3     
==========================================
- Hits        52951    52948       -3     
  Misses       9087     9087              

@@ -35,7 +35,7 @@ def make_variable(cls, compute_value, *args):
return cls(*args, compute_value=compute_value)
else:
# For compatibility with old pickles
return cls(*args)
return cls(*tuple(x for i, x in enumerate(args) if i != 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this? @lanzagar, I suppose this should be OK?

# For compatibility with old pickles: remove the second arg if it's bool
# `compute_value` (args[3]) can't be bool, so this should be safe
if len(args) >= 2 and isinstance(args[2], bool):
    args = args[:2] + args[3:]
return cls(*args)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if len(args) == 2 this raises an IndexError.

@janezd
Copy link
Contributor

janezd commented Mar 10, 2021

As much as I love these booby traps, it's annoying that all tests are failing until we merge this. Can we do it ASAP? I now proposed a workaround for the problem @lanzagar warned about on Slack.

@lanzagar, if you think it's OK, click Merge. :)

@janezd janezd added the urgent Requires immediate action label Mar 11, 2021
@janezd janezd self-assigned this Mar 12, 2021
@janezd janezd merged commit 69e580d into biolab:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
urgent Requires immediate action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants