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

Context improvements #209

Merged
merged 18 commits into from
Mar 11, 2024
Merged

Context improvements #209

merged 18 commits into from
Mar 11, 2024

Conversation

gwbres
Copy link
Collaborator

@gwbres gwbres commented Mar 6, 2024

This PR improves and simplifies the RnxContext structure.
It makes it more efficient to represent what is required in GNSS and RINEX post processing.
For example, it allows now to preprocess all types of products we support and generate new products.
It slightly reworks the way we operate the RnxContext structure and removes a big memcopy that was previously required in previous version, making the initial phase of the program more efficient.

gwbres added 9 commits March 5, 2024 13:49
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
@gwbres
Copy link
Collaborator Author

gwbres commented Mar 9, 2024

Hello @lnicola

I'm trying to improve the RnxContext structure substancially but I need your help to figure out one method specifically.

To give more context: RnxContext is not correctly defined today, in the sense it does not hold internal references to inner RINEX or SP3 files. The consequence of this, is we need to Clone() the RINEX/SP3 objects we have just parsed, when forming RnxContext, which is quite a heavy operation that you ideally want to avoid.

With this PR i'm trying to fix that, mostly by introducing a lifetime to RnxContext. I am very close to a solution.

=======================

Well I found rapidly a solution to this, it turns out the inner type must be &mut Ref.
This is no longer a problem, but I'm having a hard time providing a real usage of this new definition.
I'm not even sure that is entirely "feasible" in Rust..

@gwbres gwbres requested a review from lnicola March 9, 2024 12:44
rinex/src/context/mod.rs Outdated Show resolved Hide resolved
rinex/src/context/mod.rs Outdated Show resolved Hide resolved
@gwbres gwbres changed the title Context and other improvements Context improvements Mar 9, 2024
rinex/src/context/mod.rs Outdated Show resolved Hide resolved
gwbres added 5 commits March 9, 2024 15:25
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
@lnicola
Copy link
Member

lnicola commented Mar 10, 2024

Sorry, I didn't have a chance to look at this yesterday, and now I'm pretty confused. Even when looking 91664b2 and only the rinex crate, I get quite a lot of build errors, so I'm not sure it's the right commit:

image

@lnicola
Copy link
Member

lnicola commented Mar 10, 2024

But yeah, looking at that pattern, it might need a Rc<RefCell<..>>, which is a bit awkward to work with. Are you sure that BlobData shouldn't "own" Rinex and Sp3? Can a Rinex value be a part of more than one BlobData?

gwbres added 2 commits March 10, 2024 10:44
Signed-off-by: Guillaume W. Bres <[email protected]>
Signed-off-by: Guillaume W. Bres <[email protected]>
@gwbres
Copy link
Collaborator Author

gwbres commented Mar 10, 2024

But yeah, looking at that pattern, it might need a Rc<RefCell<..>>, which is a bit awkward to work with. Are you sure that BlobData shouldn't "own" Rinex and Sp3? Can a Rinex value be a part of more than one BlobData?

Thank you once again for all your input, it turns out it is the "natural" conclusion I came with this morning, that I have injected with the latest commits.

The conclusion is we have a solution to avoid this heavy memcopy, but it requires to slightly modify the way we operate/build/define the context at high level (in the applicatation itself)

@gwbres gwbres marked this pull request as ready for review March 10, 2024 10:19
@gwbres gwbres requested a review from larsnaesbye March 10, 2024 10:21
@gwbres gwbres merged commit e9f9838 into main Mar 11, 2024
4 checks passed
@gwbres gwbres deleted the cli-context branch March 11, 2024 07:22
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