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

Fixes for Dmitry R.'s platform #637

Closed
wants to merge 7 commits into from
Closed

Conversation

nathanwbrei
Copy link
Contributor

Briefly, what does this PR introduce?

@DraTeots helped me quite a bit with PR #578. He made a lot of small fixes in order to get EICrecon to build on his system. For the sake of keeping the huge PR as manageable as possible, I didn't include these in the final PR. Rather than throw this work away, however, I cherry-picked the changes that didn't make it into main so that we can have a look at them and decide what to keep.

Probably the most important fix in here is an issue with Eigen alignments.

The changes related to exception handling are going to inform my response to issue #625.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

@nathanwbrei nathanwbrei requested a review from DraTeots April 28, 2023 19:23
@veprbl
Copy link
Member

veprbl commented Apr 29, 2023

Can we review these changes in separate PRs? Some are not uncontroversial.

@nathanwbrei
Copy link
Contributor Author

Can we review these changes in separate PRs? Some are not uncontroversial.

Absolutely! I fully expect this us to cherry pick and/or drop some of these commits. The important thing is that we all see what the changes are, and more or less agree on which should make it into master.

When it comes to specifics:

  • Some of the changes relating to Eigen alignment look uncontroversial to me (e.g. the missing pragma once's). Although I'm pretty sure there was also some code that got commented out in there, and I don't know if that code was important.

  • Removing the redundant ParticlesWithAssociation class (and some of the many many unused headers) seems like a good idea because it would reduce code entropy and might even improve compilation times slightly.

  • The exception handling changes definitely need to be their own PR, and I am experimenting with doing it a little bit differently than what is here.

  • I have no idea what the changes to the CMake project structure entail or why we might want them. Are these the changes that you flagged as being possibly controversial?

wdconinc pushed a commit that referenced this pull request May 2, 2023
### Briefly, what does this PR introduce?

Single commit from #637. Allegedly already approved by experts.

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?

### Does this PR change default behavior?

Co-authored-by: Dmitry Romanov <romanovda@gmail.com>
@wdconinc
Copy link
Contributor

wdconinc commented Jul 4, 2023

@nathanwbrei @veprbl @DraTeots Can this be closed?

@veprbl veprbl closed this Jul 4, 2023
@veprbl
Copy link
Member

veprbl commented Jul 4, 2023

I didn't see useful changes that weren't yet ported.

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.

4 participants