mdolab / pygeo

pyGeo provides geometric design variables and constraints suitable for gradient-based optimization.
https://mdolab-pygeo.readthedocs-hosted.com/en/latest/?badge=latest
Apache License 2.0
124 stars 55 forks source link

Speed up DVJacobian computation #189

Closed marcomangano closed 1 year ago

marcomangano commented 1 year ago

Purpose

Opening this PR on behalf of @anilyil.

pyGeo currently recomputes the Jacobian of the control points w.r.t. the user-defined DVs in computeDVJacobian every time we call computeTotalJacobian. If the DVs are not updated (e.g. we are calculating derivatives on different pointSets on the same design) this step is unnecessary (and expensive) as the Jacobian remains the same. Anil added a flag self.JTempUpdated to monitor if the Jacobian is up to date with the current design. If that is the case, we skip the CS-based calculation of the DVJacobian and immediately return the current matrix. The modifications do not change the code behavior but will improve code performance, especially for more complex geometry setups.

Expected time until merged

2-3 days

Type of change

Testing

Checklist

codecov[bot] commented 1 year ago

Codecov Report

Merging #189 (92b4286) into main (41ffb0b) will increase coverage by 0.01%. The diff coverage is 91.66%.

@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   64.77%   64.79%   +0.01%     
==========================================
  Files          47       47              
  Lines       11951    11957       +6     
==========================================
+ Hits         7741     7747       +6     
  Misses       4210     4210              
Impacted Files Coverage Δ
pygeo/parameterization/DVGeo.py 68.77% <91.66%> (+0.09%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

marcomangano commented 1 year ago

The implementation looks good. My only comment is about the variable name. J_temp is fine for a temporary local variable, but I would prefer calling it something like J_DV if it is going to be a class variable.

Agreed! Pushing soon