Skip to content

Fix fromfile initial conditions - #1067

Closed
Maxwell-Rosen wants to merge 2 commits into
mainfrom
ic-fromfile-fix
Closed

Fix fromfile initial conditions#1067
Maxwell-Rosen wants to merge 2 commits into
mainfrom
ic-fromfile-fix

Conversation

@Maxwell-Rosen

Copy link
Copy Markdown
Collaborator

Claude fixed the regression test. Here is the transcript. I removed its excessive comments. The 1xIC2x2v regression test passes now.

image

�¯ Fix this bug. It was introduced when the metadata was changed. (main) gkeyll: gdb ./build/gyrokinetic/creg/rt_gk_wham_1xIC_2x2v_p1
GNU gdb (Ubuntu 17.1-2ubuntu1) 17.1
Copyright (C) 2025 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.
Type "show copying" and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for
For bug reporting instructions, please see:
<https://www.gnu.org/software/
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/softwa

For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from ./build/gIC_2x2v_p1...
(gdb) r
Starting program:/home/maxwell-rosen/gkeyll/build/gyrokinetic/creg/rt_gk_wham_1xIC_2x2v_p1
[Thread debugging using libthr
Using host libthread_db library "/usr/lib/x86_64-linux-gnu/libthread_db.so.1".

Number of update calls 1
Number of forward-Euler calls
Number of RK stage-2 failures 0
Number of RK stage-3 failures
Number of write calls 4
Timing:
- Time loop: 3.5487e-01 sec.
* Forward Euler: .18 %.
^ Collision moments (charged): 1.2653e-02 sec. / 3.75 %.
^ Reaction moments (char00 %.
^ Collision moments (neutral): 0.0000e+00 sec. / 0.00 %.
^ Reaction moments (neut00 %.
^ Radiation moments: 0.0000e+00 sec. / 0.00 %.
^ Species gyroaverage: 00 %.
^ Species LTE: 0.0000e+00 sec. / 0.00 %.
^ Collisionless terms (c.69 %.
^ Collision terms (charged): 1.8451e-01 sec. / 54.63 %.
^ Damping (charged): 00 %.
^ df/dt multiplier (charged): 0.0000e+00 sec. / 0.00 %.
^ Diffusion (charged): 00 %.
^ Radiation terms: 0.0000e+00 sec. / 0.00 %.
^ Reaction terms (charge00 %.
^ Boundary fluxes (charged): 0.0000e+00 sec. / 0.00 %.
^ omega_cfl (charged): 68 %.
^ Sources (charged): 1.5459e-03 sec. / 0.46 %.
^ BGK Sources (charged):/ 0.00 %.
^ Collisionless terms (neutral): 0.0000e+00 sec. / 0.00 %.
^ Species LTE (neutral):00 %.
^ Boundary fluxes (neutral): 0.0000e+00 sec. / 0.00 %.
^ Collision terms (neutr00 %.
^ Reaction terms (neutral): 0.0000e+00 sec. / 0.00 %.
^ omega_cfl (neutral): 00 %.
^ Sources (neutral): 0.0000e+00 sec. / 0.00 %.
^ Time step reduction: 00 %.
^ Step f: 1.8092e-03 sec. / 0.54 %.
^ Accounted for:
* Field solves: 1.3484e-02 sec. / 3.80 %.
^ Phi eqn RHS: .67 %.
^ Phi eqn solve: 1.7166e-04 sec. / 1.27 %.
^ Accounted for:
* Boundary conditions:: 2.4814e-04 sec. / 0.07 %.
^ Species (charged): 8.70 %.
^ Species (neutral): 0.0000e+00 sec. / 0.00 %.
^ Accounted for:
* Time rate diagnostics: 0.0000e+00 sec. / 0.00 %.
^ Charged species: 00 %.
^ Phi: 0.0000e+00 sec. / 0.00 %.
^ Accounted for:
* Positivity: 0.0000e+00 sec. / 0.00 %.
^ Species (charged): 00 %.
^ Species (neutral): 0.0000e+00 sec. / 0.00 %.
^ Quasineutrality: 00 %.
^ Accounted for: 100.00 %.
* Time stepper arithmetic:89 %.
* Accounted for: 100.94 %.
- I/O:
* f write (charged): 3.8466e-03 sec. / 3.55 %.
* Species diag calc (charg.32 %.
* Species diag write (charged): 7.2346e-03 sec. / 6.67 %.
* f write (neutral): 00 %.
* Species diag calc (neutral): 0.0000e+00 sec. / 0.00 %.
* Species diag write (neut00 %.
* Field write: 2.6873e-04 sec. / 0.25 %.
* Field diag calc: 02 %.
* Field diag write: 1.0598e-04 sec. / 0.10 %.
* Common write: 10 %.
* Accounted for: 100.00 %.

Program received signal SIGSEGV, Segmentation fault.
__strcmp_evex () at ../sysdeps.S:320
�� warning: 320 ../sysdeps/x86_64/multiarch/strcmp-evex.S: No such file or directory
(gdb) bt
#0 __strcmp_evex () at ../sysdeps/x86_64/multiarch/strcmp-evex.S:320
#1 0x00007ffff72f6c69 in gk_s, gks=, inp=...)
at apps/gk_species.c:1162
#2 0x00007ffff72f8d17 in gk_species_init (gk_app_inp=gk_app_inp@entry=0x7fffffcbbd58,
app=app@entry=0x555555570cps/gk_species.c:1599
#3 0x00007ffff731fea5 in gkyl_gyrokinetic_app_new_solver (gk=gk@entry=0x7fffffcbbd58,
app=0x555555570c50)
at apps/gyrokinetic.c:875
#4 0x00007ffff73209ba in gkylntry=0x7fffffcbbd58) atapps/gyrokinetic.c:976
#5 0x00007ffff733dc67 in gyron (inp=) atapps/gyrokinetic_run.c:155
#6 0x0000555555559941 in main) atcreg/rt_gk_wham_1xIC_2x2v_p1.c:1001
(gdb)
� Selected 1 lines from gyrokinetic/creg/ignore_c_tests.lua in Visual Studio Code

Searched for 10 patterns, read 3 files, listed 1 directory, ran 14 shell commands

I found the root cause. Let me confirm the exact mechanism before fixing.

The written metadata has duplicate keys: poly_order and basis_type each appear twice � once
from app->io_meta_grid (the coand once from gks->io_meta_grid (the phase-space basis, gkhybrid):

Ran 1 shell command

Confirmed. mpack treats a map with duplicate keys as mpack_error_data, which poisons the
entire parse tree � so every s� 0, basis_type � NULL) returnsits default. strcmp(gks->basis.id, NULL) then segfaults.

This was introduced by commit 761f7bd (species got its own conf+phase metadata including
poly_order/basis_type), which vel poly_order/basis_type whengk_species_write_dynamic unions both into the distribution-function file.

The right fix is in gkyl_msgpack_create_union: it concatenates arbitrary metadata lists
into one msgpack map and must make it keep the lastoccurrence of each key (later lists override earlier ones � matching the intent that the
species' phase-space basis_typs conf-space serendipity in thespecies file).

Read 1 file

� Update(core/zero/util.c)
Added 55 lines, removed 35
mpack_writer_t writer;
mpack_writer_init_g, &mdata->meta_sz);

mpack_build_map(&wr
// Flatten the union of lists so we can detect duplicate keys. MessagePac
k
// maps must have unique keys; a map with duplicates is rejected by the
// reader (mpack_erly corrupt every lookup.
int nvals_tot = 0;
for (int j=0; j<num
nvals_tot += nvals_union[j];

for (int j=0; j<numlist_union; ++j) {
const struct gkyl_mot];
int fidx = 0;
for (int j=0; j<num
for (int i=0; i<nvals_union[j]; ++i)
flat[fidx++] =

int nvals = nvals
const struct gkyl_msgpack_map_elem *elist = elist_union[j];

mpack_build_map(&wr

for (int i=0; i<n
  mpack_write_cstr(&writer, elist[i].key);

  switch (elist[i].elem_type) {
    case GKYL_MP_
      mpack_write_bool(&writer, elist[i].bval);
      break;

    case GKYL_MP_
      mpack_write_u64(&writer, elist[i].uval);
      break;

    case GKYL_MP_
      mpack_write_i64(&writer, elist[i].ival);
      break;

    case GKYL_MP_
      mpack_write_float(&writer, elist[i].fval);
      break;

    case GKYL_MP_
      mpack_write_double(&writer, elist[i].dval);
      break;

    case GKYL_MP_
      mpack_write_cstr(&writer, elist[i].cval);
      break;

    default:
      assert(false); // NYI.
      break;

for (int k=0; k<nvals_tot; ++k) {
// Keep only the later lists override earl
ier
// ones), so the plicate keys.
bool overridden = false;
for (int m=k+1; m
if (strcmp(flat[k]->key, flat[m]->key) == 0) {
overridden =
break;
}
}
if (overridden)
continue;

const struct gkyl_msgpack_map_elem *elem = flat[k];
mpack_write_cstr(

switch (elem->ele
  case GKYL_MP_BOOL:
    mpack_write_b
    break;

  case GKYL_MP_UNSIGNED_INT:
    mpack_write_u
    break;

  case GKYL_MP_INT:
    mpack_write_i
    break;

  case GKYL_MP_FLOAT:
    mpack_write_f
    break;

  case GKYL_MP_DOUBLE:
    mpack_write_d
    break;

  case GKYL_MP_STRING:
    mpack_write_c
    break;

  default:
    assert(false)
    break;
}

}

mpack_complete_map(&writer);

Now let me rebuild and verify. First I need to determine the build configuration:

Ran 1 shell command

Build succeeded. Now I need to regenerate the IC file with the fixed library, then run the
import test. The existing gk_wrrupt (duplicate-key) metadata,so let me regenerate it first:

Ran 2 shell commands

The core library wasn't rebuilt (util.c is in core/). Let me rebuild core and relink:

Read 1 file, listed 2 directories, ran 1 shell command
Found 30 new diagnostic issnd)

The fixed lib (16:13:46) is no1) predates it. Let meregenerate the IC file and run the import test:

Ran 4 shell commands

The gyrokinetic lib bundles its own copy of core objects and wasn't relinked. Let me check and force it:

@JunoRavin

Copy link
Copy Markdown
Collaborator

If you're going to post Claude conversations, can you please export the conversation to Markdown and distinguish between your prompt and the output of the agent. This whole PR is formatted strangely

@manauref

Copy link
Copy Markdown
Collaborator

I don't think we should allow posting LLM conversations as PR or DR messages. One reason is LLMs are extremely verbose, for example this PR
#997
says in 30 lines what I can say in 3.

If you expect other humans to read your contribution, I think at least the human should write the description so that it is clear and concise (i.e. respectful of other people's time).

@Antoinehoff

Copy link
Copy Markdown
Collaborator

couldn't agree more

@Maxwell-Rosen

Maxwell-Rosen commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator Author

Ammar said that he wanted entire conversations with LLMs posted with the PR if the LLM made the changes. These are the messages from Claude, and I'm just following instructions. I agree that this will increase the burden on reviewers, and it's unnecessary. I can only imagine what a multi-prompt interaction with Claude would look like in PR format.

This is just copy-pasted from the terminal. It doesn't output in a very nice format. I think there is a way to copy it though (ctrl + x ?)

@JunoRavin

Copy link
Copy Markdown
Collaborator

I am with Ammar that the LLM conversation is helpful, because we need some amount of accountability on what the LLM said and what prompt was given because I want to start holding people's feet to the fire on doing better prompting than "fix this" + screenshot and throwing the result over the fence.

So my feeling is that this PR should be rejected until it is reformatted in a way that organizes both your prompt and Claude's response, and further if you're not going to diagnose what's wrong, you need to at least prompt Claude to why this reorganization solves the issue. You said you removed Claude's excessive comments, but there may have been a kernel of utility in the looping setup. You should be making assessments of Claude's comment style beyond "delete everything" and "retain Claude's internal thought process in comment form"

@manauref

Copy link
Copy Markdown
Collaborator

"Claude fixed the regression test" in addition to a seemingly machine-written PR description makes it extremely onerous to decipher what was wrong and why the current change is needed or the suitable one for this fix. I glanced at the code changes and I couldn't immediately tell why or how, and the PR description that should've at least given me a high-level human-readable description of why and how failed to do so.

Closing this PR for now.

@manauref manauref closed this Jun 23, 2026
@Maxwell-Rosen

Copy link
Copy Markdown
Collaborator Author

I am with Ammar that the LLM conversation is helpful, because we need some amount of accountability on what the LLM said and what prompt was given because I want to start holding people's feet to the fire on doing better prompting than "fix this" + screenshot and throwing the result over the fence.

So my feeling is that this PR should be rejected until it is reformatted in a way that organizes both your prompt and Claude's response, and further if you're not going to diagnose what's wrong, you need to at least prompt Claude to why this reorganization solves the issue. You said you removed Claude's excessive comments, but there may have been a kernel of utility in the looping setup. You should be making assessments of Claude's comment style beyond "delete everything" and "retain Claude's internal thought process in comment form"

I think you're contradicting yourself here. You want claude's prompt and response, but you want it in an organized and readable way. Claude does give the reason it finds that this change is made, but it is buried in layers of text. Yes, I agree that PRs should not be structured like this, and this is an example of how it becomes unreadable when we copy-paste LLM output.

I have also lost my Claude conversation, so if I didn't copy-paste it here, I wouldn't be able to post it. Furthermore, I find that practice is unsustainable over multiple-prompt sessions. My proposal is to simply state how many LLMs were used and acknowledge responsibility for their changes. A statement of LLM usage, then why the fix is necessary, is a much better, readable PR. The human contributor should offer their two cents on why the PR is necessary or what the fix was in this case.

Even in this case, copy-pasting the prompt is verbose, and that's half of the PR text.

@Antoinehoff
Antoinehoff deleted the ic-fromfile-fix branch June 23, 2026 14:46
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