-
Notifications
You must be signed in to change notification settings - Fork 47
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
petab: Enable Importer passing verbose to create_model #1269
Conversation
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1269 +/- ##
===========================================
- Coverage 84.59% 82.73% -1.86%
===========================================
Files 148 148
Lines 12112 12112
===========================================
- Hits 10246 10021 -225
- Misses 1866 2091 +225 ☔ View full report in Codecov by Sentry. |
pypesto/petab/importer.py
Outdated
@@ -428,7 +428,10 @@ def create_objective( | |||
|
|||
# create model | |||
if model is None: | |||
model = self.create_model(force_compile=force_compile) | |||
verbose = kwargs.pop('verbose', True) |
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.
It's good to be able to pass verbose=False
. However, it's potentially confusing to do that via kwargs
(**kwargs: Additional arguments passed on to the objective.
). I'd prefer adding (and documenting) it as a separate keyword argument to create_objective
.
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.
True, will change.
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.
Additionally, I extended the same thing to all methods of the PetabImporter
(create_edatas, create_solver etc...). To be uniform in changes, but it might be too much?
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.
To be uniform in changes, but it might be too much?
No strong opinion. I don't think it will be used frequently via those other functions, but I don't think it hurts having it there.
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.
Same feeling
Added verbose as argument in all importer method which use `create_model` and passed it onto it.
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.
Thx
Enable the
create_objective
andcreate_problem
methods of thePetabImporter
to pass verbose tocreate_model
andsbml2amici
. Coming from #1246