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

Update ValidationManifold #217

Merged
merged 31 commits into from
Dec 9, 2024
Merged

Conversation

kellertuer
Copy link
Member

@kellertuer kellertuer commented Nov 23, 2024

Aims to resolve #216.

🛣️🗺️

  • introduce a point to the vector validation types
  • introduce s store_base_point to ValidationManifold
  • add a fine granular dictionary to disable certain validations
  • adapt all existing methods
  • adapt all error calls to now use _msg to also respect the M.mode in these
  • check documentation that the goal of the validation manifold becomes clearer
  • check that also newer interface functions are covered by the validation manifold
  • fix converts / ambiguities after generalising to accepting all points for validation

@kellertuer kellertuer marked this pull request as draft November 23, 2024 17:16
Copy link

codecov bot commented Nov 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (e116010) to head (51dbfa6).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #217      +/-   ##
==========================================
+ Coverage   99.94%   99.97%   +0.03%     
==========================================
  Files          31       31              
  Lines        3401     3535     +134     
==========================================
+ Hits         3399     3534     +135     
+ Misses          2        1       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kellertuer
Copy link
Member Author

@mateuszbaran can you maybe take a look at the current design of the idea of ignoring parts of the tests?
I just tested it on distance, since there we have a “non point, non vector output” that potentially still has a check.

But I already tried documenting it fully as well

If this design is ok I would go through the whole API and

  • add functions to the validation manifold that are currently not yet covered
  • add these deactivation mechanisms to all checks performed.

Copy link
Member

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I have two comments for now.

src/ValidationManifold.jl Outdated Show resolved Hide resolved
src/ValidationManifold.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

I found even a nicer way

  • overload is_point(M::ValidationManifold,...) with two keywords – the context :Point is also added automatically there.
  • pass M also to _vMc

the internals stay within _vMc that way

I renamed the vector-thing to ignore_context because for example with :Input as an area/context to ignore, this sounded good.

Now one only has to put a function=..., context=... to a lot of the input/output checks.

@kellertuer
Copy link
Member Author

Just adapted all other point/vector checks to this new schemes, feels good in style I think.

  1. I want to rework the error calls to use _msg (which might turn these into warnings) and adapt them to the function/context modes as well.
  2. I want to check which functions from the API are not yet covered in the validation and add them as well.

Besides these two points, the PR should be done, so I already set it to active.

@kellertuer kellertuer marked this pull request as ready for review December 1, 2024 14:25
@kellertuer kellertuer added the Ready-for-Review A label for pull requests that are feature-ready label Dec 2, 2024
@kellertuer
Copy link
Member Author

This is feature complete and hence ready for review.

  • Since now all old “errors” that always threw errors now respect both mode and context, several tests have still to be adapted
  • Since I also added a few functions from the interface, these also still need test coverage.

Will continue with this the next few days. But overall I like the new features and how they turned out

@mateuszbaran
Copy link
Member

Nice, I think the updated approach looks better.

@kellertuer
Copy link
Member Author

The original tests are back up and running, just one convert from a manifold to a variation manifold is broken and I can not see how to fix it – in other words: If that code worked before I do not see why it does now no longer work.

The remainder will just be to also cover the new functions I implemented.

@mateuszbaran
Copy link
Member

The main reason why the old test doesn't work is that that == in Julia is comparing by default in a non-intuitive way when a struct has mutable fields. See here: https://discourse.julialang.org/t/surprising-struct-equality-test/4890 . Previously there were no mutable fields in ValidationManifold and now there are.

@kellertuer
Copy link
Member Author

Ah, I thought it was only the convert. Sure the new one has mutable fields.

Thanks for the fix! Then just the tests for the new functions are missing – and maybe a bit about vectors of symbols. But that is basically clear how to do that.

@kellertuer
Copy link
Member Author

Now just

  • embed embed_project
  • riemannian_tensor
  • and a few text message prints (which is rare now since most are Errors) are missing.

NEWS.md Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

I hope this PR should now be finished.

@kellertuer
Copy link
Member Author

This PR'does not yet introduce interlinks, but the CSS for the prefixes is also useful here as well already, so I felt adding it wouldn't be too bad. But with that the PR is indeed finished.

src/ValidationManifold.jl Outdated Show resolved Hide resolved
@kellertuer
Copy link
Member Author

I fixed the addition, I hope now all works fine as intended also with base points in TVectors.

@kellertuer
Copy link
Member Author

I noticed I also had to do the update for mutating cases. So let's see how much coverage is missing now.
Besides the tests the PR should be finished.

@kellertuer kellertuer merged commit 76d6a5f into master Dec 9, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready-for-Review A label for pull requests that are feature-ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve ValidationManifold
2 participants