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

Ohmms matrix assignment operator should be able to do a widening assigment #5145

Merged
merged 2 commits into from
Aug 26, 2024

Conversation

PDoakORNL
Copy link
Contributor

Proposed changes

Right now the Matrix class can only do a widening or narrowing assignment if the Matrices involved have the same dimensions. I don't think this makes much sense and its quite problematic for how I want to write the hdf5 output from the new estimators. To do that hdf5 output only widening assignments are made i.e. a Matrix is assigned to a Matrix.

#5139 depends on this PR to fix its CI failures.

However there are number of narrowing assigments in the mixed precision all adhere to this matching size rule. Maybe that's an important constraint? So I have left this behavior. But I do think that probably it should be changed so all Matrix Matrix assignments have the same semantics.

Important to note that through the magic of PETE all the non matching assignments were going through the same template operator=. And I've just added a overload to hand assignment from a narrower Matrix.

What type(s) of changes does this code introduce?

  • New feature
  • Testing changes (e.g. new unit/integration/performance tests)
  • Documentation or build script changes

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

x86 v100

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes This PR is up to date with current the current state of 'develop'
  • Yes Code added or changed in the PR has been clang-formatted
  • Yes This PR adds tests to cover any new code, or to catch a bug that is being fixed
  • Yes Documentation has been added (if appropriate)

@PDoakORNL PDoakORNL requested a review from ye-luo August 26, 2024 17:22
@PDoakORNL
Copy link
Contributor Author

The codecov failure looks like its checking the coverage of the test_Matrix.cpp which is undesirable. I'm having trouble even getting the coverage build to link (issue with my own environment and curl). Should I write an issue for this? It does seem to be a failure with compare_gcov.py but I shouldn't go down that rabbit hole this week.

inline This_t& operator=(const This_t& rhs)
{
resize(rhs.D1, rhs.D2);
// I don't understand why it would be desirable to have this compile
// but just do the resize and not the assigment, just seems like a surprising foot gun.
Copy link
Contributor

Choose a reason for hiding this comment

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

A good catch. Users can abuse it for making matching matrices without the need of copying over values. Such use cases should not call operator = .
Needs a revisit when we scrutinize the operator = overloading.

@ye-luo
Copy link
Contributor

ye-luo commented Aug 26, 2024

The codecov failure looks like its checking the coverage of the test_Matrix.cpp which is undesirable. I'm having trouble even getting the coverage build to link (issue with my own environment and curl). Should I write an issue for this? It does seem to be a failure with compare_gcov.py but I shouldn't go down that rabbit hole this week.

https://app.codecov.io/gh/QMCPACK/qmcpack/pull/5145?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=checks&utm_campaign=pr+comments&utm_term=QMCPACK

It seems indicating so. We have a 'ignore' setting

- "src/*/*/tests"

but I don't know how it interacts with codecov.

@ye-luo
Copy link
Contributor

ye-luo commented Aug 26, 2024

Test this please

@ye-luo ye-luo enabled auto-merge August 26, 2024 17:50
@ye-luo ye-luo merged commit 1e0af53 into QMCPACK:develop Aug 26, 2024
37 of 39 checks passed
@prckent
Copy link
Contributor

prckent commented Aug 26, 2024

Thanks Peter. This is a good catch. The assignment operator as resize but no assignment looks like a old design bug; there could be an unintentional use lurking.

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