Remove non-functional QMCPACK inputs - #6184
Conversation
# Conflicts: # nexus/nexus/qmcpack_input.py
# Conflicts: # nexus/nexus/qmcpack_input.py
|
Did you scan through all the XML files in the repository and remove the use of those XML tags you removed in qmcpack? |
PDoakORNL
left a comment
There was a problem hiding this comment.
The agent dropped the input and initial usage of the input of a working feature from SpinDensity while leaving behind the now much larger amount of dead implementation code. It could do this because my unit testing would have only broken if it removed the rest of the code, I did not protect the code path with only implicit DataLocality through the input.
That said, it does look like to me that the functionality is broken at the application level currently and there is no coverage of that or sufficient integration testing with EstimatorManagerNew. So next I will write an issue for that and put that in my queue.
I don't know how this was done, I'm just going to put out there that the bigger the context a model deals with at once the sloppier things can get. I don't know how this was done precisely but I have personally seen the reasoning quality really fall off past 30%. Many agent harnesses only compact when context is at 80% or more, Claude code often runs it all the way out to overflow and then backtracks. I don't know what codex does at the moment. On non interactive long running tasks I think more slop happens.
I've been working small more and challenging changes along the way that seem questionable. They can be hard to see when you are looking at 100's of files touched.
| parameters = {"dr", "grid", "corner", "center", "cell"}; | ||
| strings = {"name"}; | ||
| bools = {"report", "save_memory"}; | ||
| bools = {"report"}; |
There was a problem hiding this comment.
save_memory does do something uses a different memory pattern for moves that for large grids reduces the size of memory needed on the walkers.
| species_size_(getSpeciesSize(species)) | ||
| { | ||
| data_locality_ = DataLocality::crowd; | ||
| if (input_.get_save_memory()) |
There was a problem hiding this comment.
This removes working functionality, I don't think that's the intention and all the other memory saving code is left behind by this as dead code.
| { | ||
| #if OHMMS_DIM == 3 | ||
| std::string a("ion0"), targetName("e"), title("ForceBase"), pbc("yes"), PsiName = "psi0"; | ||
| std::string a("ion0"), targetName("e"), title("ForceBase"), pbc("yes"); |
There was a problem hiding this comment.
This looks like PsiName just lost it's default, that could have consequences.
|
@PDoakORNL Thanks for the comments. Keep putting more here and I will address them. The process I used was multi-pass with many intermediate documents created that contained key information before moving on to a new subtask. I used these documents to refresh the context in places that warranted it and performed additional cross-reference checks. For example: I can provide the entire conversation transcript if desired. |
|
Any further review? I think the vast majority of the removals are correct. |
|
I will likely take a look next week after recovering from the PI meeting. This PR raises more interesting questions than originally expected. I would like to understand why removing non-functional inputs requires so many added lines and understand that we are not building up more work for us in future when legacy is blocked (and when we are paying more for AI assistance). I am also wondering how many of these tests are actually worthwhile vs just hitting coverage but not being an effective test. Maybe it is better to block legacy first, saving the future work of removing the added tests here (?). |
This PR makes an extensive effort to identify and remove QMCPACK input variables that do not drive or connect to existing functionality in the code.
Total removals are summarized below:
Files detailing the specific removals (one line per element/attribute/parameter removed):
qmcpack_vars_removed_legacy.txt
qmcpack_vars_removed_batched.txt
As part of the process, the full deterministic integration test suite was boiled down to 11 overloaded tests. For each, functional-only and functional+nonfunctional versions where used. It was first verified that the functional+nonfunctional input files gave the same deterministic results as the functional+nonfunctional ones.
The full unit test suite and the broadly updated deterministic integration tests all pass.
GPT-Sol Ultra and Extra High were used exclusively.
This PR follows on from PR 6164. I recommend merging that PR first before inspecting the contents of this PR in any detail.
Line counts unique to this PR:
Quite a few of the added tests check for non-functionality of the relevant inputs. Since these are no longer in the code following this PR, and any future reintroductions are highly likely to be intentional, it is probably worthwhile to remove those tests. If this is desired before review, let me know. This would update the total added/removed lines to +561/-1417.