Feature/adaptive truncation by RH-MikkelWerling · Pull Request #70 · FGA-DIKU/ehr2meds

RH-MikkelWerling · 2026-07-28T19:33:19Z

Trying to implement adaptive truncation in the pipeline

coderabbitai · 2026-07-28T19:33:28Z

Warning

Review limit reached

@RH-MikkelWerling, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 58 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: dd83db35-4f98-4327-8428-ab772ce5e5a3

📥 Commits

Reviewing files that changed from the base of the PR and between dc4b531 and e5a229d.

📒 Files selected for processing (13)

.gitignore
README.md
configs/MEDS/default_adaptive_code_mapping.yaml
configs/MEDS/lymphoma_pipeline.yaml
configs/MEDS/lymphoma_pipeline_external_mapping.yaml
ehr2meds/adaptive_code_mapping.py
ehr2meds/io_utils.py
ehr2meds/meds_stages/annotate_numeric_values.py
ehr2meds/meds_stages/apply_adaptive_code_mapping.py
ehr2meds/meds_stages/bin_numeric_values_fast.py
ehr2meds/meds_stages/finalize_adaptive_code_metadata.py
ehr2meds/meds_stages/fit_adaptive_code_mapping.py
pyproject.toml

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feature/adaptive_truncation

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

mikkelfo

Generally looks fine, but again I'm worried about the amount of code added for these features. The more we add, the harder it is to maintain imo.

We obviously require some functionality so we cannot aimlessly minimise, but we're generally adding a high level of verbosity to these functions that we may never look at. I think it's a fine line we have to walk (@Sllambias thoughts?)

mikkelfo · 2026-07-29T08:43:10Z

I'm a little worried about the level of verbosity for the columns. Seems like a lot of code for an extensive level of verbosity (via different columns) that we may never look at

True, agree - I've made much less checks and balances now. For writing up the code though, it is much easier to get a sense of what's up and down going overboard with how verbose it is. I've made it much more manageable now, totally agree with the sentiment.

Sllambias · 2026-07-29T09:22:46Z

+from pathlib import Path
+
+CODE_COLUMN = DataSchema.code_name
+MAPPED_CODE_COLUMN = "adaptive/mapped_code"


I think this should just be a default in the main function calling for this, or be contained in a yaml

Yeah, moved it.

Sllambias

Agree with mikkel on the verbosity. I think the explanations make sense, but I think they're better placed in readme's that can be directly referred to from the yaml files and then I had a few comments. Happy to look again at a later time aswell

Sllambias · 2026-07-29T09:23:14Z

+from MEDS_transforms.utils import PKG_PFX, resolve_pkg_path
+from pathlib import Path
+
+CODE_COLUMN = DataSchema.code_name


Preferably just referred to as DataSchema.code_name in the code

Agree, this again - opted for more DataSchema.code_name in the code instead.

Sllambias · 2026-07-29T09:24:24Z

+    return resolve_pkg_path(filepath) if filepath.startswith(PKG_PFX) else Path(filepath)
+
+
+def load_frame(filepath: str, label: str) -> pl.DataFrame:


Is this something that should be a more generally shared function or does it necessarily need to be nested here?
As opposed to being in a more general function file that is used by all of EHR2meds - it seems rather general maybe apart from path = resolve_resource_path(filepath)

Yeah, super good point. This actually simplified things. I've added a io_utils.py now with a few helpers, and I think that setup makes more sense.

Sllambias · 2026-07-29T09:25:45Z

+    external_mapping_mode: str,
+) -> pl.DataFrame:
+    """Select a local mapping and optionally overlay or replace it externally."""
+    required_local = {CODE_COLUMN, MAPPED_CODE_COLUMN}


can you explain the different scenarios here? I'm trying to figure out if support all of these are necessary and can't really tell

Like what is the case where

required_local is not subset of local_metadata and external_mapping_filepath is given

required_local is not subset of local_metadata and external_mapping_filepath is not given

required_local is subset of local_metadata and external_mapping_filepath is not given

required_local is subset of local_metadata and external_mapping_filepath is given

Yeah, super good question.
So, going through this, I rewrote much of the code because some of the instances don't really make sense.
Like for instance, you can imagine having to fit on your own data and not needing external data (say for instance, when you are training PHAIR light). The behavior before was also a bit messy in that it would add metadata for codes that were not in the external data, which is why it needed both. Now this can technically happen - you could imagine having a mismatch between data sources, where an upstream model doesn't include some data source that does have a hierarchy you want to truncate. However, I figured that this would be so rare that we are better off opting for a system where you can specify the stages for the pipeline that calculates the aggregate measures and thereby bin / truncate based on your own data, or otherwise use an external source as a backup, which is then taken as ground truth and not messed with. The other behavior was a mistake on my part; I thought it was smart, but I think it's stupid.
This also means that the behavior now is simpler. It doesn't cover as many different combinations, but it's easier to understand and more aligned with what the actual use cases are.

Sllambias · 2026-07-29T09:30:30Z

+    if mode != "overlay":
+        raise ValueError("external_mapping_mode must be 'overlay' or 'replace'")
+    return pl.concat([external, local]).unique(CODE_COLUMN, keep="first", maintain_order=True)


Very minor suggestion but I think this is slightly nicer

Suggested change

if mode != "overlay":

raise ValueError("external_mapping_mode must be 'overlay' or 'replace'")

return pl.concat([external, local]).unique(CODE_COLUMN, keep="first", maintain_order=True)

if mode == "overlay":

return pl.concat([external, local]).unique(CODE_COLUMN, keep="first", maintain_order=True)

else:

raise ValueError(f"external_mapping_mode must be 'overlay' or 'replace' but is {mode}")

Sllambias · 2026-07-29T09:32:58Z

I think this is a really nice file, and fits perfectly with what I also wanted to do with the other "tests" running in the workflows.

I'm a little concerned about (my perceived) complexity of these tests and I fear they may be prone to breaking quite easily and being relatively hard to maintain for maintainers (like me) not too familiar with the exact usage. But this is just a concern and may be completely wrong.

yeah it was super useful for developing, but I think generally these files should be wiped after development or be integrated in a larger test suite. Don't think they make sense on their own, so I'll probably delete it now.

Sllambias · 2026-07-29T09:41:51Z

+    """
+    cfg = OmegaConf.to_container(stage_cfg, resolve=True)
+    namespaces = {str(namespace): str(profile) for namespace, profile in cfg["namespaces"].items()}
+    hierarchies = cfg.get("hierarchies", {})


what happens when hierarchies is empty?

made it so it errors instead of defaulting to an empty set

Sllambias · 2026-07-29T09:46:55Z

+    }
+
+
+def fit_mapping(


maybe this function should be split into two

tried but didn't see a super good way of doing it. There are parts of the loop that needs previous parts of the code, so a natural split is kind of hard to make I think. Happy to take a second look if you see a path forward!

RH-MikkelWerling added 2 commits July 28, 2026 21:04

first try at adaptive truncation

cdd7106

Polish adaptive mapping audit output

4951099

RH-MikkelWerling added 4 commits July 29, 2026 09:29

remove sks mapping

5b08b0d

move functionality and only keep levels functionality

402286e

add default values config

dedb197

update fit_adaptive_code_mappings

55a3adc

RH-MikkelWerling mentioned this pull request Jul 29, 2026

FEATURE: Adaptive truncation #69

Open

mikkelfo reviewed Jul 29, 2026

View reviewed changes

mikkelfo requested a review from Sllambias July 29, 2026 08:50

Sllambias reviewed Jul 29, 2026

View reviewed changes

RH-MikkelWerling added 11 commits July 29, 2026 16:13

add claude to gitignore

f323559

remove tracking for settings.json

18b8d3b

remove temporary alias

fbd0684

declutter finalize adaptive

30a7f4d

make hierarchies empty result in error

cd14345

remove default

0bf9acd

move explanations to readme

48d9c19

make utils and restructure

5f2353e

revert to simpler handling of external aggregation data

19a7d87

test functionality

34a6ecf

ruff is ruff

08ce42b

RH-MikkelWerling marked this pull request as ready for review July 30, 2026 14:08

remove test file

e5a229d

		return resolve_pkg_path(filepath) if filepath.startswith(PKG_PFX) else Path(filepath)


		def load_frame(filepath: str, label: str) -> pl.DataFrame:

Uh oh!

Conversation

RH-MikkelWerling commented Jul 28, 2026

Uh oh!

coderabbitai Bot commented Jul 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

mikkelfo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sllambias left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 28, 2026 •

edited

Loading