ethz-asl / data-driven-dynamics

Data Driven Dynamics Modeling for Aerial Vehicles
Other
91 stars 13 forks source link

Separate loading dataframes and data selection #226

Closed Jaeyoung-Lim closed 1 year ago

Jaeyoung-Lim commented 1 year ago

Problem Description This PR separates the selection of dataframes and loading of dataframes

Jaeyoung-Lim commented 1 year ago

After making this PR, rethinking whether the previous state was better :smile: @manumerous @sjschlapbach what do you think?

sjschlapbach commented 1 year ago

After making this PR, rethinking whether the previous state was better 😄 @manumerous @sjschlapbach what do you think?

@Jaeyoung-Lim Thanks for the fix! 😄 I think the previous state led to a significant speed-up of the setpoint-based data selection as it was not required to parse the entire ulog file and load it to the model, whenever only certain parts of the log should be used. However, I remember that we discussed some potential issues arising from these changes regarding the data used for the fisher information matrix computation?

Jaeyoung-Lim commented 1 year ago

@sjschlapbach Hmm...but there should be no real saving of computation time since it is not doing less or more for all cases right?

sjschlapbach commented 1 year ago

@Jaeyoung-Lim Ah, I think you're right - I confused something there, sorry... In this case, I think, it depends on the data for which the Fisher information should be computed, right? My thought behind the previous changes was that we might want to compute the informativity metrics only for the data sections we actually use in the cases where we make use of the setpoint-based selection. And I think it was the computation of the regression matrices, which took significantly less time when only using certain sections of the log :)

Probably, it comes down to a specification then. If I don't miss anything, the regression results / coefficients should be the same in both cases as the regression matrices are overwritten anyway?

Jaeyoung-Lim commented 1 year ago

@sjschlapbach Kept the previous changes, but just fixed the missing fisher information calculation!