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

[Fitting] Expose barycenter and add barycenterLocal #141

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Conversation

nmellado
Copy link
Contributor

@nmellado nmellado commented Jul 10, 2024

In the current version it is not clear that the barycenter is given in local coordinate system.

Todo

  • changelog
  • double-check documentation
  • methods visibility (local should be for internal use only)

@nmellado nmellado force-pushed the fix_barycenter branch 2 times, most recently from 60d0fc4 to 3d64252 Compare July 10, 2024 18:46
@nmellado
Copy link
Contributor Author

This problem has been identified by @wgoadri

Copy link

@dcoeurjo dcoeurjo left a comment

Choose a reason for hiding this comment

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

Oh I see. Not sure to see the side effects of this edit but the local/global clarification is valuable per se.

Would that affect some estimators Leo is testing?

Copy link
Contributor

@azaleostu azaleostu left a comment

Choose a reason for hiding this comment

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

Seems good to me. I agree that the old barycenter was unclear whether it was in local or global space. I also agree that code using the old barycenter might break, but it should be fine since the change is documented.

@nmellado
Copy link
Contributor Author

It is true that this is a breaking change that will lead to a major release bump (btw we might want to merge it after #143 then).
I checked all internal use. I don't think this will affect downstream code too much, as this function has been introduced recently and is probably not used that much. We will make that clear in the release notes.

@nmellado nmellado force-pushed the fix_barycenter branch 2 times, most recently from 1d33be0 to e65a731 Compare July 10, 2024 20:31
@nmellado
Copy link
Contributor Author

I updated the changelog to highlight the breaking change.

@nmellado
Copy link
Contributor Author

Just pushed again to fix a bug when rebasing: I was using old barycenter() instead of new barycenterLocal() in the tests.

@nmellado nmellado merged commit 1610535 into master Jul 11, 2024
10 of 12 checks passed
@nmellado nmellado deleted the fix_barycenter branch July 11, 2024 13:20
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