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

Minor fixups ensembles #568

Merged
merged 9 commits into from
Feb 22, 2021
Merged

Minor fixups ensembles #568

merged 9 commits into from
Feb 22, 2021

Conversation

paulstapor
Copy link
Contributor

Making the predictor more flexible and fixing a bug in the parameter mapping...

Copy link
Member

@dilpath dilpath left a comment

Choose a reason for hiding this comment

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

👍

pypesto/petab/importer.py Outdated Show resolved Hide resolved
pypesto/prediction/amici_predictor.py Outdated Show resolved Hide resolved
Comment on lines 305 to +310
for rdata in chunk[RDATAS]:
amici_outputs.append({AMICI_STATUS: rdata[AMICI_STATUS],
AMICI_T: rdata[AMICI_T],
AMICI_X: rdata[AMICI_X],
AMICI_SX: rdata[AMICI_SX],
AMICI_Y: rdata[AMICI_Y],
AMICI_SY: rdata[AMICI_SY]})
amici_outputs.append({
output_field: deepcopy(rdata[output_field])
for output_field in self.amici_output_fields
})
del chunk
Copy link
Member

Choose a reason for hiding this comment

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

Is deepcopy used because chunk remained in memory (or I guess, because after del chunk, maybe the value in amici_outputs is also deleted without deepcopy)? If so, is both deepcopy and del necessary? Since chunk should get garbage collected I guess, if there are no references to it? Just curious, fine to leave as-is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually intended t go without the deepcopy and the del, and it seemed to work fine. However, in my debugger I experienced that chunk was still present i nthe memory at a later call of the function and just got overwritten... That puzzled me extremely...
Might be, that this is really only due to the debugger. However, I wanted make really, really safe that chunk is dead, dead, DEAD! 🦇 No, not undead, dead. ⚰️ ... That's why deepcopy and del... 🤷‍♂️

paulstapor and others added 2 commits February 19, 2021 21:36
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
@codecov-io
Copy link

codecov-io commented Feb 21, 2021

Codecov Report

Merging #568 (134eed3) into develop (3e765d9) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #568      +/-   ##
===========================================
- Coverage    88.57%   88.56%   -0.01%     
===========================================
  Files           79       79              
  Lines         5243     5257      +14     
===========================================
+ Hits          4644     4656      +12     
- Misses         599      601       +2     
Impacted Files Coverage Δ
pypesto/petab/importer.py 85.89% <ø> (ø)
pypesto/ensemble/dimension_reduction.py 83.33% <100.00%> (ø)
pypesto/ensemble/ensemble.py 80.76% <100.00%> (ø)
pypesto/objective/amici.py 89.83% <100.00%> (+0.11%) ⬆️
pypesto/prediction/amici_predictor.py 97.53% <100.00%> (+0.30%) ⬆️
pypesto/prediction/constants.py 100.00% <100.00%> (ø)
pypesto/prediction/prediction.py 94.44% <100.00%> (+0.12%) ⬆️
pypesto/sample/geweke_test.py 91.54% <0.00%> (-2.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e765d9...134eed3. Read the comment docs.

@paulstapor paulstapor merged commit 160c2a8 into develop Feb 22, 2021
@dilpath dilpath deleted the minor_fixups_ensembles branch February 22, 2021 20:31
@yannikschaelte yannikschaelte mentioned this pull request Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants