Skip to content

fix: strip duplicated resolution frame validation#208

Closed
nikbpetrov wants to merge 2 commits into
forecastingresearch:mainfrom
nikbpetrov:fix-duplicated-res-frame-validation
Closed

fix: strip duplicated resolution frame validation#208
nikbpetrov wants to merge 2 commits into
forecastingresearch:mainfrom
nikbpetrov:fix-duplicated-res-frame-validation

Conversation

@nikbpetrov

Copy link
Copy Markdown
Collaborator

fixes #206

also happens to fix a dormant deviation for infer: original infer refactor stripped infer's astype(dtype=constants.RESOLUTION_FILE_COLUMN_DTYPE) when building the resolution frame. Is now corrected and matches manifold/metaculus.

Let me know if I should run parity tests for all 3 sources - I will assume not.

@nikbpetrov nikbpetrov requested a review from houtanb June 8, 2026 08:54
@nikbpetrov nikbpetrov force-pushed the fix-duplicated-res-frame-validation branch from 07129d2 to b01480b Compare June 10, 2026 07:15
@nikbpetrov

Copy link
Copy Markdown
Collaborator Author

@houtanb now also fixed polymarket as it was duplicating validation too but I'd also like to ask your advice on whether I should just refactor the semantics of resolution file building altogether - I don't like it anymore.

Some sources have both _build_resolution_file as well as _finalize_resolution_df, while others don't (seems like a data/market source split). I don't see the point in that - we don't use/centralize these abstractions. I'd suggest we stick to only _build_resolution_df that each source (optionally) has and all the stuff inside _finalize_resolution_df (for infer/manifold/metaculus) just gets moved to _build_resolution_df (which is the rename of _build_resolution_file.

What do you think?

@houtanb houtanb force-pushed the fix-duplicated-res-frame-validation branch from b01480b to 11e3324 Compare June 10, 2026 07:32
Comment thread src/sources/infer.py
@@ -386,14 +386,14 @@ def _build_resolution_file(

@staticmethod
def _finalize_resolution_df(df: pd.DataFrame) -> DataFrame[ResolutionFrame]:

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.

Should this return pd.DataFrame now that validation has moved to UpdateResult.__post_init__? Currently DataFrame[ResolutionFrame] implies this helper returns a validated schema.

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.

Same question for:

    def _build_resolution_file(
        self,
        question: dict,
        resolved: bool,
        existing_df: DataFrame[ResolutionFrame] | None = None,
    ) -> DataFrame[ResolutionFrame]:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I want to keep the annotation for readability reasons. I appreciate the implicitness of the annotation but it's not decorated with @pa.check_types so validation does not apply.

@nikbpetrov

Copy link
Copy Markdown
Collaborator Author

#215

@nikbpetrov nikbpetrov closed this Jun 10, 2026
houtanb pushed a commit that referenced this pull request Jun 15, 2026
houtanb pushed a commit that referenced this pull request Jun 15, 2026
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.

Strip duplicated resolution frame validation

2 participants