New result reader with delegation to existing and consolidated reader #385
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
efredriksson-modelon
requested review from
PeterMeisrimelModelon and
hugo-sigurdson
PeterMeisrimelModelon
requested changes
Jan 13, 2026
Collaborator
PeterMeisrimelModelon
left a comment
PeterMeisrimelModelon
left a comment
There was a problem hiding this comment.
A few comments, largely around documentation, changes look good!
Also, I miss some test using ResultReaderBinaryMat for a simulation results produced using dynamic_diagnostics.
Also wondering if we should have some type of regression tests on the actual instances of delegates as to properly test/verify the different if branches in delegation selection.
efredriksson-modelon
force-pushed
the
dev-ef-mat-load-delegation
branch
from
20f9ee1 to
1a8adba
Compare
Collaborator
Author
|
@PeterMeisrimelModelon Thanks for the review. I have added new commits for the changes (and missed force push for the docstrings). I will squash things before merging. |
PeterMeisrimelModelon
approved these changes
Jan 13, 2026
efredriksson-modelon
force-pushed
the
dev-ef-mat-load-delegation
branch
from
87b8625 to
f1ea564
Compare
…te either to 'default' or new consolidated implementation docs: fix missing args fix: remove delayed_trajectory_loading argument from ResultReaderBinaryMat chore: add clarifying comment on when using which result reader test: add check that the delegator works when simulating with dynamic diagnostics
It is not possible to re-use the data loaded for both cases of delegation. The reason is they use different implementations of the delayed loading which returns different results, what is expected to be the full data vs a file position pointer. For example 'data_3' is different for the two implementations. So, instead of trying to re-use it for one of the implementations we can instead try to minimize the amount of data handling for doing the delegation. This is done by using the delayed loader for the consolidated result and add support to it to not load in any data_4.
efredriksson-modelon
force-pushed
the
dev-ef-mat-load-delegation
branch
from
f1ea564 to
f5d5543
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This MR adds the main result reader that can delegate.
It is was not possible to re-use the data loaded during delegation setup for both cases of delegation. The reason is they use different implementations of the delayed loading which returns different results. For example what is expected to be the full data vs a file position pointer, 'data_3' is different for the two implementations in this way.
So, instead of trying to re-use it for one of the implementations we can instead try to minimize the amount of data read during the delegation setup. This is done by using the delayed loader for the consolidated result and add support to it to not load in any data_4 (we only need to ask for 'data_3' and 'data_4' other data will then be ignored entirely).