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

Caching Free Integrated Aesara Objective #629

Merged
merged 8 commits into from
Apr 13, 2021
Merged

Conversation

FFroehlich
Copy link
Contributor

@FFroehlich FFroehlich commented Apr 12, 2021

Simplifies the construction of aesara objectives. This implementation allows exploiting integrated evaluation of the wrapped objective.

@codecov-io
Copy link

codecov-io commented Apr 12, 2021

Codecov Report

Merging #629 (476dcee) into develop (160c2a8) will decrease coverage by 53.83%.
The diff coverage is 23.73%.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop     #629       +/-   ##
============================================
- Coverage    88.16%   34.33%   -53.84%     
============================================
  Files           79       93       +14     
  Lines         5257     5884      +627     
============================================
- Hits          4635     2020     -2615     
- Misses         622     3864     +3242     
Impacted Files Coverage Δ
pypesto/ensemble/covariance_analysis.py 18.36% <0.00%> (-0.39%) ⬇️
pypesto/objective/aesara.py 0.00% <0.00%> (ø)
pypesto/objective/aggregated.py 25.49% <0.00%> (-74.51%) ⬇️
pypesto/petab/__init__.py 0.00% <0.00%> (ø)
pypesto/petab/importer.py 0.00% <0.00%> (-85.90%) ⬇️
pypesto/petab/pysb_importer.py 0.00% <0.00%> (-100.00%) ⬇️
pypesto/visualize/parameters.py 14.14% <0.00%> (-77.78%) ⬇️
pypesto/visualize/waterfall.py 14.81% <0.00%> (-81.49%) ⬇️
pypesto/store/read_from_hdf5.py 51.32% <5.55%> (-6.57%) ⬇️
pypesto/store/save_to_hdf5.py 18.96% <7.69%> (-43.18%) ⬇️
... and 100 more

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 e955dc7...476dcee. Read the comment docs.

Copy link
Member

@yannikschaelte yannikschaelte left a comment

Choose a reason for hiding this comment

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

looks very nice and useful!, don't get one point yet.

pypesto/objective/aesara.py Show resolved Hide resolved

return ret

def __deepcopy__(self, memodict=None):
Copy link
Member

Choose a reason for hiding this comment

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

is serialization of aesara objects possible?

Copy link
Member

Choose a reason for hiding this comment

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

(fine without too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 haven't tested it though.

Comment on lines +161 to 164
log_prob = self._coeff * self._objective.inner_ret[FVAL]
outputs[0][0] = np.array(log_prob)

def grad(self, inputs, g):
Copy link
Member

Choose a reason for hiding this comment

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

howby what dark magic is it possible that inputs is not used here? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note about this. The trick is that we already know what the inputs here are going to look like when computing inner_ret, so we can ignore them here and just use the precomputed values. This also has the advantage that we only spend a minimal amount of time in aesara functions, since the bulk of computation is done in the outer call_unprocessed, which improves performance in multithreaded settings as we avoid a lot of locking of aesara functions.

pypesto/objective/aesara.py Outdated Show resolved Hide resolved
Fabian Fröhlich and others added 2 commits April 13, 2021 10:15
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
@FFroehlich FFroehlich merged commit 789b04a into develop Apr 13, 2021
@FFroehlich FFroehlich deleted the feature_cached_aesara branch April 13, 2021 14:39
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