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.

Sign up for GitHub

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

Jump to bottom

refactor: move math functionality from epee to src #7856

Closed
wants to merge 1 commit into from
Closed

refactor: move math functionality from epee to src #7856

wants to merge 1 commit into from

Conversation

UkoeHB
Copy link
Contributor

@UkoeHB UkoeHB commented Aug 12, 2021 •
edited

Summary

  • Moves math functions median() and rolling_median_t from epee to src. These two things are not being used outside of src.
  • Moves unit test coverage of median() into unit_tests/. Previously, as noted in CMake epee: add minimalistic test setup +CI #7854, the test for median() that lived in epee was not being compiled or executed by CI.
  • The math header is called math_utils.h instead of math.h due to an obscure compile issue.
  • Note: The get_mid<T>() function remains in epee because it is needed by epee's get_median() (PR epee: overflow bugfix in get_median() #7858).

This PR was inspired by @vtnerd's comment that epee is focused on network-related things, so math stuff should go elsewhere.

This PR conflicts with #7854, so I am happy to hear advice on the best way forward.

Future PRs

  • Add an n_choose_k math function to support a different future PR (for multisig key gen).
  • Add unit test coverage of get_mid().

@mj-xmr
Copy link
Contributor

mj-xmr commented Aug 14, 2021 •
edited

This PR conflicts with #7854, so I am happy to hear advice on the best way forward.

I don't mind. I use math there only as an example.
But honestly I question a bit the need of moving the math outside epee. Especially since more influential code moves are being rejected in this project.

Epee is not only network related code.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Aug 15, 2021 •
edited

I think it would be helpful to have a discussion about the future of epee. What should epee look like, and how can this PR be one step in the right direction?

@mj-xmr
Copy link
Contributor

mj-xmr commented Aug 15, 2021

I think it would be helpful to have a discussion about the future of epee. What should epee look like, and how can this PR be one step in the right direction?

I will just review the techicals through (soon) and leave the decision to others.

@UkoeHB UkoeHB mentioned this pull request Aug 18, 2021
Merged
mj-xmr
mj-xmr reviewed Aug 23, 2021
View reviewed changes
src/math/math_utils.h
{

template<class type_vec_type>
type_vec_type median(std::vector<type_vec_type> &v)
Copy link
Contributor

@mj-xmr mj-xmr Aug 23, 2021 •
edited

Choose a reason for hiding this comment

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

It't not related to this PR, but this parameter should really be a copy, not a reference. I don't expect a function, that returns a median, to sort my vector. It will even not work at all, if the passed vector is const, forcing the caller to be non-const-correct in his higher context.

mj-xmr
mj-xmr reviewed Aug 23, 2021
View reviewed changes
src/math/CMakeLists.txt Outdated Show resolved Hide resolved
mj-xmr
mj-xmr reviewed Aug 23, 2021
View reviewed changes
src/math/math.cpp
// THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

#include "math_utils.h"
#include "rolling_median.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion this is correct, since this helps in syntax checks of templates' declarations before the parses leaves the component.

mj-xmr
mj-xmr reviewed Aug 23, 2021
View reviewed changes
src/math/math_utils.h
{//2, 4, 6...
return epee::misc_utils::get_mid<type_vec_type>(v[n-1],v[n]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

No unintended changes spotted while moving the code.

mj-xmr
mj-xmr reviewed Aug 23, 2021
View reviewed changes
Copy link
Contributor

@mj-xmr mj-xmr left a comment •
edited

Choose a reason for hiding this comment

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

Except for small changes needed for CMakeLists.txt, the technicals are alright.

The copyright change is an issue IMO.

mj-xmr
mj-xmr suggested changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@mj-xmr mj-xmr left a comment

Choose a reason for hiding this comment

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

Copyright changed

src/math/math_utils.h Outdated Show resolved Hide resolved
mj-xmr
mj-xmr reviewed Aug 24, 2021
View reviewed changes
src/math/CMakeLists.txt Outdated Show resolved Hide resolved
mj-xmr
mj-xmr suggested changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@mj-xmr mj-xmr left a comment

Choose a reason for hiding this comment

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

Just that small nitpick in CMakeLists.txt an it's ready for me.

mj-xmr
mj-xmr approved these changes Aug 24, 2021
View reviewed changes
Copy link
Contributor

@mj-xmr mj-xmr left a comment

Choose a reason for hiding this comment

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

You will be asked for squashing the commits before the branch can be merged. I'm not sure myself when the best time for this is.

mj-xmr
mj-xmr approved these changes Aug 25, 2021
View reviewed changes
Copy link
Contributor

@mj-xmr mj-xmr left a comment •
edited

Choose a reason for hiding this comment

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

Re-reviewed the squashed commit.
All green.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Oct 9, 2021

Rebased (maybe not necessary in hindsight)

@selsta
Copy link
Collaborator

selsta commented Oct 11, 2021

Conflicts with #7819, so IMO it's best to wait with merging this.

@jeffro256 jeffro256 mentioned this pull request Mar 10, 2022
Merged
@UkoeHB
Copy link
Contributor Author

UkoeHB commented Dec 12, 2022

Follow up: it might be fine to move the math stuff to the tools namespace in src/common.

@UkoeHB
Copy link
Contributor Author

UkoeHB commented Feb 27, 2023

I'm going to close this PR. I decided to just put new math utils like n_choose_k() in my seraphis library under src/seraphis_crypto/math_utils.*. It would be nice to have this PR merged, but it's not going anywhere and it's not that important.

@UkoeHB UkoeHB closed this Feb 27, 2023
@UkoeHB UkoeHB deleted the refactor_math branch February 27, 2023 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@selsta selsta selsta left review comments

@mj-xmr mj-xmr mj-xmr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@UkoeHB @mj-xmr @selsta