Add missing batched deterministic integration tests - #6164
Conversation
github-actions
Bot
added
nexus
cmake
python
|
Thanks for working on this. It will be interesting if this actually shows any more coverage in CI, i.e. missing line vs logical coverage. Since the deterministic tests are user facing I think this PR can wait until after the release. One immediate request: after #6153, none of the deterministic test inputs have "short" in their names. You might need to rerun with that PR merged in. Also, after recent updates, backflow is mostly working. There may be a few other missed updates from the last few days (?). Have you experimented with asking for test values? In my experience this should "just work" for the 2x2 CPU build combinations (mixed & complex {0,1}), but I have not tried the whole test set. There is also the offload case. We want to track where the numbers are different from legacy. In many cases we can use the exact same values. We also need to be very careful with marking tests as "expect fail" since we need to track what functionality is missing, even if it is headed for the wood chipper. |
|
If the corresponding batched driver test passes, I'd rather drop the legacy and keep only the batched. Developers are burdened by tests. Maybe introduce cmake option to allow some tests switchable between legacy and batched. At least we don't need to introduce more input files. |
|
Extra comment: chopping non-functional input variables is something we can do today, independent of the batched transition. There are quite a few lurking from 10+ years ago. |
|
@ye-luo I am regenerating the reference values directly (automated). @prckent I have detailed code traces identifying the likely non-functional inputs. There are lots and lots of these. I will use these deterministic tests, along with existing ones and unit tests, to ferret out the ones that can be removed without somehow damaging existing execution. |
|
@jtkrogel I meant running extra tests are burden. |
|
Interestingly it looks like the checks won't run automatically until the conflict is fixed. "Checks awaiting conflict resolution". |
|
Regenerated reference data and value comparison tests added. |
|
This PR is ready to be reviewed. Once passing review, I will update against develop and we can confirm passing tests. I would like to request that desires for added bells and whistles be checked - this PR is focused solely on adding the new batched tests with parity to the existing legacy ones. |
There was a problem hiding this comment.
You will need to update the deterministic inputs to avoid short in their names to avoid duplication. There are lots of "new" files here that will not actually be new when updated to develop. Rerunning the workflow is likely easiest and lowest effort because e.g. reference files were also renamed and scripts updated in this process. This is additionally important because while I think the heg_14_gamma changes will disappear since we already have batched tests, the new batched heg_52_J2rpa test will remain. I spent hours on this part of the repo recently so am familiar. My memory of other parts of the repo will not be as fresh.
Please also fix the conflict so the CI runs. As I mentioned, it will be interesting what happens to the test coverage.
# Conflicts: # nexus/nexus/qmcpack_input.py
|
@prckent Let me know if the file changes addressed your concerns Also, I see failures in the CI, but the causes aren't immediately clear. Are these just tolerance issues or something else? |
|
I would normally say "click for detail" but GitHub has been acting up today. But they do appear to be failures in the added tests that will need fixing. If samples are wrong this is not a tolerance issue. |
PDoakORNL
left a comment
PDoakORNL
left a comment
There was a problem hiding this comment.
I agree it would be nice if we could control the scope of the generated tests i.e. not add the legacy ones by default. But I think the better thing to do is just delete them ASAP, can't we just assert that while all legacy code is not yet removed, it is the process of being removed, is not supported and increasingly not tested?
| static_assert(std::is_copy_constructible_v<OneBodyDensityMatricesInput>); | ||
| } | ||
|
|
||
| TEST_CASE("OneBodyDensityMatricesInput::volume_normed", "[estimators]") |
There was a problem hiding this comment.
Here we didn't have coverage that a particular input string really results in the correct native input representation. I think generating more comprehensive tests like this should get on the road map. I will look into doing it. In the meantime "volume_normed" will at least not have a regression.
Unfortunately It doesn't look to me like its actually tested, i.e. volume_normed=no doesn't appear covered by an application or unit level test. So while this isn't "unsupported" we don't protect this functionality from breakage and I can't attest it works as intended. Am I missing something?
If true this should become an issue it's not a defect with the PR.
There was a problem hiding this comment.
I don't think volume_normed is covered. It is not on the updated "unsupported" list and will be kept through the removal process.
|
As far as deleting legacy, etc. Is the upcoming release the last to support a working legacy implementation? If so, a quick following turnaround would be to rip out all the legacy tests. Easily done. |
|
CI is just hanging? |
|
The GitHub hosted CI actions have passed. The self-hosted (ornl-*) CI only runs when we ask it to. Ask if you would like it to be run. There is a conflict to fix which will require all the CI to be run post-fix. |
|
It is fine with me if the additional CI is not run. I will fix the conflict. |
|
I think it's ready now. |
|
Test this please |
|
Noting that when we have bandwidth this needs discussion on the path forward. We do not want to accidentally mark something as having a passing deterministic test when it is incorrect. If/how much this applies needs bandwidth to assess. |
|
I will make an issue detailing the very few "should fail" tests that need targeted fixes in the main source. |
|
Just list them here for now. Less bureaucracy and we can decide what to do based on what they are. |
|
I checked in more detail. The issues I dug up were resolved by #6149, so nothing to do. |
|
@jtkrogel all the failing tests seems just need to nudge up the tolerance slightly. |
|
The adjustments just made should be sufficient. |
|
Test this please |
# Conflicts: # nexus/nexus/qmcpack_input.py
4 participants
This PR includes rollovers of deterministic integration tests present for the legacy drivers but missing for the batched drivers.
The current commit checks only for successful completion -- all pass except for ones involving backflow.
Backflow tests have been marked as expected to fail. I assume that the functionality is simply broken and needs to be fixed later.
These tests will next be updated to include checks against generated reference values.
This effort is a step along the way to removing non-functional input variables.
GPT 5.6 Sol was used with levels ranging from High to Ultra depending on the sub-task.