Fix get_required_parameters_for_parameter_table
#340
Conversation
The previous check did not make any sense. See discussion at PEtab-dev#339 (comment).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #340 +/- ##
========================================
Coverage 74.65% 74.65%
========================================
Files 54 54
Lines 5381 5381
Branches 923 923
========================================
Hits 4017 4017
Misses 1012 1012
Partials 352 352 ☔ View full report in Codecov by Sentry. |
| # for ALL conditions | ||
| for p in condition_df.columns[~condition_df.isnull().any()]: | ||
| # parameters that are overridden via the condition table are not allowed | ||
| for p in condition_df.columns: |
There was a problem hiding this comment.
Instead of try-except, could do one of these, but fine as is
for p in set(condition_df.columns).intersection(parameter_ids):
del parameter_ids[p]
for p in condition_df.columns:
if p in parameter_ids:
del parameter_ids[p]There was a problem hiding this comment.
Right, but then one has to look up each element twice.
| ~problem.condition_df.isnull().any() | ||
| ]: | ||
| # parameters that are overridden via the condition table are not allowed | ||
| for p in problem.condition_df.columns: |
There was a problem hiding this comment.
Since parameter IDs is now a set, can change try-except/loop to
parameter_ids -= set(condition_df.columns)There was a problem hiding this comment.
Right, but parameter_ids is still an OrderedDict (we wanted to preserve order here). Only the returned DictKeyView (or similar) acts like a set.
There was a problem hiding this comment.
Since it's right before returning, it could be done just on the keys, but then future modifications might easily mess up the ordering. I'd leave it as is until Python has a proper OrderedSet.
There was a problem hiding this comment.
Where does it become OrderedDict? In the v1 code it's initialized as an OrderedDict, but here as a set?
There was a problem hiding this comment.
You are right. I was in v1.
Not sure why I changed it to set there in the first place.
The previous check did not make any sense. See discussion at #339 (comment).
Also fix a missing import.