Skip to content

Kylie bug fix - #15

Merged
mirams merged 14 commits into
masterfrom
kylie_bug_fix
May 27, 2026
Merged

mirams merged 14 commits into
masterfrom
kylie_bug_fix

Conversation

@mirams

@mirams mirams commented May 26, 2026 •

Copy link
Copy Markdown
Member

To close #14

Copilot AI review requested due to automatic review settings May 26, 2026 00:29
@mirams mirams linked an issue May 26, 2026 that may be closed by this pull request
@mirams
mirams requested review from kwabenantim and removed request for Copilot May 26, 2026 00:29
@mirams

mirams commented May 26, 2026 •

Copy link
Copy Markdown
Member Author

If you could test locally @kwabenantim and check it does sort out #14 that would be great.

I have tried and failed, but suspect it is me having a stale docker around or something, as code seems plausible.

Comment thread server.js Outdated
Copilot AI review requested due to automatic review settings May 26, 2026 00:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses Issue #14 by preventing ApPredict from receiving an invalid --pacing-max-time argument when pacingMaxTime is not provided in the input JSON, and updates some user-facing text/copyright headers.

Changes:

  • Build ApPredict CLI args so --pacing-max-time is only included when pacingMaxTime is present.
  • Improve the invalid-operation error message (spacing + include STDERR/STDOUT).
  • Update copyright years in help text and LICENSE.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
server.js Avoids passing undefined pacing max time to ApPredict; updates GET operation error messaging.
LICENSE Updates copyright year range.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server.js Outdated
mirams and others added 2 commits May 26, 2026 01:40
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@kwabenantim

Copy link
Copy Markdown
Member

After the changes, voltage_results are now:

{"success":[{"c":"Concentration(uM)","uv":"UpstrokeVelocity(mV/ms)","pv":"PeakVm(mV)","a50":"APD50(ms)","a90":"APD90(ms)","da90":["delta_APD90(%)"]},
{"c":"0","uv":"350.596","pv":"47.8243","a50":"184.455","a90":"216.892","da90":["0"]},
{"c":"0.001","uv":"351.022","pv":"47.8407","a50":"184.7","a90":"217.155","da90":["0.121071"]}]}

@kwabenantim kwabenantim left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@kwabenantim

Copy link
Copy Markdown
Member

@copilot resolve the merge conflicts in this pull request, keep both the scripts and the engine version

Copilot AI commented May 26, 2026

Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request, keep both the scripts and the engine version

Resolved the merge conflicts in commit 0776c02. I kept both the scripts block and the engines.node version constraint in package.json, and completed the merge with upstream changes.

Copilot AI requested a review from kwabenantim May 26, 2026 20:26
@mirams

mirams commented May 27, 2026

Copy link
Copy Markdown
Member Author

I don't think this is working yet, but still not convinced. For request.json of

{
    "modelId": 1,
    "pacingFrequency": 0.5,
    "plasmaPoints": [
        0,
        3,
        10,
        30,
        100
    ]
} 

I got :

{"success":[{"c":"Concentration(uM)","uv":"UpstrokeVelocity(mV/ms)","pv":"PeakVm(mV)","a50":"APD50(ms)","a90":"APD90(ms)","da90":["delta_APD90(%)"]},
{"c":"0","uv":"350.595","pv":"47.8241","a50":"184.455","a90":"216.892","da90":["0"]},
{"c":"0.001","uv":"351.024","pv":"47.8408","a50":"184.7","a90":"217.155","da90":["0.121071"]},
{"c":"3","uv":"351.389","pv":"47.8557","a50":"184.92","a90":"217.391","da90":["0.22991"]},
{"c":"10","uv":"351.708","pv":"47.8692","a50":"185.12","a90":"217.605","da90":["0.328427"]},
{"c":"30","uv":"351.972","pv":"47.8814","a50":"185.3","a90":"217.798","da90":["0.417461"]},
{"c":"100","uv":"352.204","pv":"47.8923","a50":"185.464","a90":"217.973","da90":["0.497991"]}]}

which isn't right, there is still a --pacing-max-time undefined in the middle of http://localhost:8080/api/collection/463d1fd5-43f2-4c01-ac5b-f905140a95d7/STDOUT

It should basically have zeros in the da90 entries.

@kwabenantim

Copy link
Copy Markdown
Member

@mirams I've now added in a test that rebuilds the image and checks that the args in STDOUT exactly match

ApPredict args : --pacing-freq 0.5 --plasma-concs 0 3 10 30 100 --model 1

@mirams

mirams commented May 27, 2026

Copy link
Copy Markdown
Member Author

Hmmm, what are the full voltage_results with that?

@kwabenantim

kwabenantim commented May 27, 2026 •

Copy link
Copy Markdown
Member

@mirams the CI now prints out the results as well:
https://github.com/CardiacModelling/ap-nimbus-app-manager/actions/runs/26507615582/job/78064006131?pr=15

In the last run, the voltage_results were:

Full voltage_results:
 [
  {
    "c": "Concentration(uM)",
    "uv": "UpstrokeVelocity(mV/ms)",
    "pv": "PeakVm(mV)",
    "a50": "APD50(ms)",
    "a90": "APD90(ms)",
    "da90": [
      "delta_APD90(%)"
    ]
  },
  {
    "c": "0",
    "uv": "353.889",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "0"
    ]
  },
  {
    "c": "0.001",
    "uv": "353.89",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "-5.91879e-05"
    ]
  }
] 

@mirams

mirams commented May 27, 2026

Copy link
Copy Markdown
Member Author

Thanks, I think that's a different simulation, different concs, is it easy for you to test with the request in my comment above?

@kwabenantim

Copy link
Copy Markdown
Member

@mirams The tests are now waiting a bit longer (allowing up to 5m) for the full results to complete.

The final voltage_results are:
https://github.com/CardiacModelling/ap-nimbus-app-manager/actions/runs/26510620273/job/78073907795?pr=15

Full voltage_results:
 [
  {
    "c": "Concentration(uM)",
    "uv": "UpstrokeVelocity(mV/ms)",
    "pv": "PeakVm(mV)",
    "a50": "APD50(ms)",
    "a90": "APD90(ms)",
    "da90": [
      "delta_APD90(%)"
    ]
  },
  {
    "c": "0",
    "uv": "353.889",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "0"
    ]
  },
  {
    "c": "0.001",
    "uv": "353.89",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "-5.91879e-05"
    ]
  },
  {
    "c": "3",
    "uv": "353.891",
    "pv": "47.9939",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "7.21773e-05"
    ]
  },
  {
    "c": "10",
    "uv": "353.89",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "-6.13187e-05"
    ]
  },
  {
    "c": "30",
    "uv": "353.889",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "2.5213e-05"
    ]
  },
  {
    "c": "100",
    "uv": "353.89",
    "pv": "47.9938",
    "a50": "186.989",
    "a90": "219.604",
    "da90": [
      "-3.50691e-05"
    ]
  }
] 

@mirams

mirams commented May 27, 2026

Copy link
Copy Markdown
Member Author

Perfect, that looks fixed.

@mirams
mirams merged commit bc8d56b into master May 27, 2026
2 checks passed
@mirams
mirams deleted the kylie_bug_fix branch May 27, 2026 14:37
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.

Strange behaviour when no max pacing time specified

4 participants