-
Notifications
You must be signed in to change notification settings - Fork 7
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
Simulator: rename measurement column to simulation #199
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #199 +/- ##
===========================================
+ Coverage 76.25% 76.28% +0.03%
===========================================
Files 34 34
Lines 3155 3159 +4
Branches 764 765 +1
===========================================
+ Hits 2406 2410 +4
Misses 552 552
Partials 197 197
|
That.
Maybe we should split it into a Simulator and a SyntheticDataGenerator, each with their natural default?
Fair point, but I'd say it is not so widely used yet, that I'd rather avoid confusing future users. I would change |
Looks good to me, though i of course would prefer to have the default for rename set to |
petab/simulate.py
Outdated
@@ -115,6 +115,7 @@ def simulate( | |||
self, | |||
noise: bool = False, | |||
noise_scaling_factor: float = 1, | |||
rename: bool = False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you consider changing this to True
?
Thanks for the feedback! I opted to default to The simulator now enforces the column name, such that the tool (e.g. AMICI/COPASI) naming choice of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to specify/document what simulate_without_noise
is supposed to return - measurement
or simulation
.
To me it would feel most natural to have simulation
for no-noise, and measurement
for with-noise. I don't know if this might be considered confusing by others.
Hm, at the moment the PEtab spec states
But this looks like a typo, since the Currently, the docstring for
But the
Currently, as user can choose the column name, with |
Agreed, should be changed.
Sorry, I missed that. I accidentally looked at
No, I think it's clearly specified then. |
What's your opinion on whether to default to
measurement
orsimulation
? Either is fine for me. Currently I am in favor of keepingmeasurement
, because:However, I see that
simulation
might be what users expect from a simulator.see also: AMICI-dev/AMICI#2050 and AMICI-dev/AMICI#2051