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

Grab changes from the review of #1152 (chord #1146) #1167

Merged
merged 27 commits into from
Mar 21, 2024
Merged

Grab changes from the review of #1152 (chord #1146) #1167

merged 27 commits into from
Mar 21, 2024

Conversation

dstndstn
Copy link
Contributor

@dstndstn dstndstn commented Feb 1, 2024

Cherry-pick 99c2ee9 from dstn/develop/chord-1146: "address Andre's review: add various comments, and pull out FEngine.hpp header"

@dstndstn
Cherry-pick 99c2ee9 from dstn/develop/chord-1146: "address Andre's re…
7daf9de 
…view: add various comments, and pull out FEngine.hpp header"
@andrerenard
Copy link
Member

@dstndstn do you also want me to review this, or was there something you wanted Erik to check?

@dstndstn
Copy link
Contributor Author

dstndstn commented Mar 4, 2024

Looks like this has gone a bit stale, let me resolve the conflicts and then yes, if you could review that would be great!

@dstndstn
Copy link
Contributor Author

Hi @andrerenard , if you wouldn't mind reviewing this, that would be great!

andrerenard
andrerenard approved these changes Mar 19, 2024
View reviewed changes
Copy link
Member

@andrerenard andrerenard left a comment

Choose a reason for hiding this comment

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

Approved, with one comment to consider.

tests/boost/test_ringbuffer.cpp Outdated Show resolved Hide resolved
@dstndstn
Copy link
Contributor Author

The test failures look really unrelated...

@andrerenard
Copy link
Member

I'm going to try rerunning them. We do have some tests that fail due to random number generator bad luck... But odd that all of them seemed to fail...

@dstndstn
Copy link
Contributor Author

The values seem to be very far off, so doesn't look like one of these random-sample things, maybe it's a real structural change we made by accident, but is it happening on the current head of the chord branch?

@andrerenard
Copy link
Member

andrerenard commented Mar 19, 2024 •
edited
Loading

Oh yeah, some of those values are way out... Could the location settings somehow impact this? Seems really unlikely. I'm not seeing where this could have been added in the current head. The last big PR to go in seemed to pass everything except the boost tests.

@andrerenard
Copy link
Member

Best guess would be something updated in the docker container, since we've been making a few changes to that. @jbmertens Would you able to try and figure out what's happening here? It's in the N2 pipeline, and I think something you've looked at recently?

@jbmertens
Copy link
Contributor

Best guess would be something updated in the docker container, since we've been making a few changes to that. @jbmertens Would you able to try and figure out what's happening here? It's in the N2 pipeline, and I think something you've looked at recently?

Yeah, this does look like something that changed in Docker. The test passes on an older docker build from # 1152. I'll try to see exactly what changed.

@dstndstn
Copy link
Contributor Author

Okay, I think this is finally ready for review again!!

andrerenard
andrerenard requested changes Mar 21, 2024
View reviewed changes
lib/stages/visTestPattern.cpp Show resolved Hide resolved
tests/boost/test_utils.hpp
// https://www.boost.org/doc/libs/1_75_0/libs/test/doc/html/boost_test/tests_organization/fixtures/global.html
// Enable this in your test suite by adding:
// BOOST_TEST_GLOBAL_FIXTURE(GlobalFixture_Locale);
struct GlobalFixture_Locale {
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, can we just remove this if we aren't going to use it anymore?

Copy link
Contributor Author

@dstndstn dstndstn Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yes, definitely could. I was setting the locale explicitly (to classic()) in kotekan.cpp, so I figured it would be good to at least have the mechanism to do the same in the tests, even if we're not using it. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds reasonable.

@dstndstn dstndstn merged commit b07048c into chord Mar 21, 2024
14 checks passed
@dstndstn dstndstn deleted the chord-1152 branch March 21, 2024 19:08
eschnett added a commit that referenced this pull request Mar 22, 2024
@eschnett
Merge branch 'chord' into eschnett/performance
0c0052b 
* chord:
  Use ringbuffer & metadata in cudaCorrelator kernel (#1188)
  try to fix github action when merging back to chord (#1193)
  Grab changes from the review of #1152 (chord #1146) (#1167)

# Conflicts:
#	lib/stages/FEngine.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrerenard andrerenard andrerenard approved these changes

@eschnett eschnett Awaiting requested review from eschnett

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
@dstndstn @andrerenard @jbmertens