v2: Observable/conditions/experiments/... objects#345
v2: Observable/conditions/experiments/... objects#345dweindl merged 12 commits intoPEtab-dev:developfrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #345 +/- ##
===========================================
- Coverage 74.83% 74.50% -0.33%
===========================================
Files 56 57 +1
Lines 5603 6002 +399
Branches 982 1039 +57
===========================================
+ Hits 4193 4472 +279
- Misses 1036 1133 +97
- Partials 374 397 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
dweindl
force-pushed
the
object_model
branch
5 times, most recently
from
6e7f91d to
fca15e4
Compare
dweindl
force-pushed
the
object_model
branch
2 times, most recently
from
9ba7fa2 to
b7b8b5b
Compare
dilpath
left a comment
dilpath
left a comment
There was a problem hiding this comment.
Looks good! I reviewed this without considering the other PEtab v2 things, i.e. I only checked that the OOP code look good, since I guess those other things are out-of-scope here.
| OT_CUR_VAL = "setCurrentValue" | ||
| OT_NO_CHANGE = "noChange" |
There was a problem hiding this comment.
Other constants in C.py are not shortened, so could stick to that "convention". To reduce the length, OT/OPERATION_TYPE could be dropped from the start of the name, since I don't think we have that style elsewhere either.
There was a problem hiding this comment.
I would prefer introducing prefixes elsewhere than dropping them. I find that this makes it clearer where they belong.
There was a problem hiding this comment.
Also fine to completely replace them by enums.
There was a problem hiding this comment.
I'll leave it for now, as there is a good chance that "operation types" will be dropped completely
| arbitrary_types_allowed = True | ||
|
|
||
|
|
||
| class ObservablesTable(BaseModel): |
There was a problem hiding this comment.
I guess this PR is independent of any decision on choice between "observable table" and "observables table" naming style, or is this the decision?
There was a problem hiding this comment.
Independent. Happy to rename just about anything here after the v2 specs are finalized.
| class OperationType(str, Enum): | ||
| """Operation types for model changes in the PEtab conditions table.""" | ||
|
|
||
| # TODO update names | ||
| SET_CURRENT_VALUE = "setCurrentValue" | ||
| NO_CHANGE = "noChange" | ||
| ... |
There was a problem hiding this comment.
Same with this, can replace the "same" thing in C.py
There was a problem hiding this comment.
There is a lot of repeated code (e.g. from/to_dataframe/tsv, and add/iadd). Is it nicer/possible to somehow only define these once?
There was a problem hiding this comment.
I expect that some additional validation will happen there in the future which would make it rather difficult to implement that only once. I think the classes will me more understandable if they have their own add/iadd instead of some complex function combining the logic of all the different types.
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
3 participants
Implement an object model for the various PEtab entities as an alternative to working with the plain DataFrames.
Related to #337.
Object names are very likely to change. This is more of a proof of concept.
Also adapt to recent changes in PEtab-dev/PEtab#581