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

Default getSample methods for group manifolds #1439

Merged
merged 2 commits into from
Nov 13, 2021

Conversation

Affie
Copy link
Member

@Affie Affie commented Nov 1, 2021

No description provided.

@codecov
Copy link

codecov bot commented Nov 1, 2021

Codecov Report

Merging #1439 (f80fa36) into master (adbe12a) will decrease coverage by 0.14%.
The diff coverage is n/a.

❗ Current head f80fa36 differs from pull request most recent head 75b4d88. Consider uploading reports for the commit 75b4d88 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1439      +/-   ##
==========================================
- Coverage   77.18%   77.04%   -0.15%     
==========================================
  Files          67       67              
  Lines        4940     4948       +8     
==========================================
- Hits         3813     3812       -1     
- Misses       1127     1136       +9     
Impacted Files Coverage Δ
src/ManifoldSampling.jl 66.66% <ø> (-24.25%) ⬇️
src/NumericalCalculations.jl 91.57% <0.00%> (-1.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adbe12a...75b4d88. Read the comment docs.

@Affie Affie requested a review from dehann November 3, 2021 16:26
@dehann
Copy link
Member

dehann commented Nov 4, 2021

From the call today, two comments came up:

  • Check hasproperty for .Z and give good warning for user to easily understand
  • Lets use convention of field .Z as the automated field for both non-parametric and parametric (non-restrictive),

xref #1441

Added to requirements wiki: https://github.com/JuliaRobotics/Caesar.jl/wiki/High-Level-Requirements

@dehann dehann modified the milestones: v0.0.x, v0.26.0 Nov 4, 2021
@Affie
Copy link
Member Author

Affie commented Nov 6, 2021

@dehann, comments implemented.

@Affie Affie requested a review from dehann November 6, 2021 09:44
@Affie
Copy link
Member Author

Affie commented Nov 6, 2021

I don't think this is a breaking change, should it not be in v0.25.6?

function getSample(cf::CalcFactor{<:AbstractPrior})
M = getManifold(cf.factor)
if hasfield(typeof(cf.factor), :Z)
Copy link
Member

@dehann dehann Nov 8, 2021

Choose a reason for hiding this comment

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

perhaps we should standardize with API

function getMeasurement(::AbstractFactor) ...

to return the field .Z etc. This can then also be used the existing function getMeasurementParametric which extracts the mean and covariance of the same .Z for parametric solves?

@dehann dehann merged commit e69fe83 into master Nov 13, 2021
@dehann dehann linked an issue Nov 15, 2021 that may be closed by this pull request
2 tasks
@dehann dehann deleted the 21Q4/enh/default_getSample branch December 27, 2022 06:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

default factors to .Z for automation (and docs)
2 participants